qwik icon indicating copy to clipboard operation
qwik copied to clipboard

fix(server-functions): only throw SSR guard when 'window' is undefined

Open Matt-Williams1 opened this issue 8 months ago • 8 comments

What is it?

  • Bug

Description

Previously the SSR‐guard in server-functions.ts unconditionally threw an error whenever isServer was true, which blocked invocation of routeAction$ under Vitest+JSDOM (and the QwikCityMockProvider) in user tests.

Now we narrow the guard to:

if (isServer && typeof window === 'undefined') { throw …; }

After this fix:

  • True SSR(no 'window') still throws an error as before
  • JSDOM/Vitest tests(and browsers) have a global 'window', do not throw the error, and returns a promise

This will unblock testing of actions in JSDOM environments without impacting real SSR safety

Closes #5874

Checklist

Matt-Williams1 avatar May 16 '25 16:05 Matt-Williams1

🦋 Changeset detected

Latest commit: 3b2b2f76e663521db7260531bda11488b63686c7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@builder.io/qwik-city Patch
eslint-plugin-qwik Patch
@builder.io/qwik Patch
create-qwik Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar May 16 '25 16:05 changeset-bot[bot]

Open in StackBlitz

npm i https://pkg.pr.new/@builder.io/qwik@7606
npm i https://pkg.pr.new/@builder.io/qwik-city@7606
npm i https://pkg.pr.new/eslint-plugin-qwik@7606
npm i https://pkg.pr.new/create-qwik@7606

commit: eaff266

pkg-pr-new[bot] avatar May 16 '25 16:05 pkg-pr-new[bot]

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview 35c8b3ae22565cc8984dc08c6fcda069e99c0aae

github-actions[bot] avatar May 16 '25 16:05 github-actions[bot]

I don't understand - why would isServer be true in this case?

wmertens avatar May 16 '25 17:05 wmertens

@wmertens

After digging into the code, I found that Qwik’s isServer flag is simply !isBrowser under the hood (see packages/qwik/src/build/index.ts. Essentially, it checks for window/HTMLElement) and is set to true whenever code is first run in SSR mode—even in Vitest’s JSDOM environment.

The original guard in server-functions.ts threw an error whenever isServer was true. Since Qwik treats the initial, un-hydrated pass under JSDOM as “SSR,” that guard also fired in Vitest tests and blocked actions there.

To resolve this, I refined the condition so we only throw errors in a real server environment (when there is no window global):

if (isServer && typeof window === 'undefined') {
  throw new Error(/* ... */);
}
  • Real SSR (Node, no window) still errors as before.
  • JSDOM/Vitest and browsers (where window exists) skip the throw and return a Promise.

To validate this solution, In a quick Node REPL test(via replit), I simulated both scenarios by defining and deleting global.window:

image

Result: image

With this change, all existing Playwright E2E tests remain green, and Vitest+JSDOM tests now pass without needing further mocks.

Let me know your thoughts and feedback. I'm always happy to have a discussion about this and find better potential solutions!

Matt-Williams1 avatar May 16 '25 21:05 Matt-Williams1

@LogProphet what I don't understand is that isServer can be true when it's not supposed to be.

It's a build constant that means the code is meant to be running on the server. So why is code that was meant for server side running client things?

I think this indicates a bug.

wmertens avatar May 16 '25 21:05 wmertens

@wmertens

It definitely is indicative of a bug. That's why I referenced the bug in my commit and PR description 🙃 #5874

let me clarify where the confusion comes from and why the change is necessary:

  1. isServer doesnt mean “no DOM right now,” it's defined as: “this is the SSR bundle.” Qwik (via Vite) compiles two builds: one for SSR and one for the browser. During the initial render—whether on your real server or inside Vitest’s JSDOM—Qwik runs the SSR build, so isServer === true even when JSDOM(Vitest) provides a window.

  2. That SSR build still needs a way to detect “real” server versus “simulated” server. Without our extra check, the guard for isServer will throw an error in both cases, blocking actions in any environment that uses the SSR bundle (including tests).

  3. The change im making narrows the guard to only trip when there’s truly no DOM:

if (isServer && typeof window === 'undefined') { throw new Error(/* … */); }

Real server (Node) has no window → guard still throws.

JSDOM tests (SSR code + fake window via Vitest) skip the throw and allow the action to run.

In short, the patch doesn’t change what counts as “SSR build,” it just adds a DOM–presence check so that running the SSR bundle inside a JSDOM(Vitest) environment no longer trips the guard. This keeps real-SSR protections intact while unblocking our Vitest+JSDOM test suite.

..does that make sense within the context of the linked bug? Let me know if you have any questions, or if you think im off-base here!

Matt-Williams1 avatar May 16 '25 22:05 Matt-Williams1

aha, I think the problem stems from build-time-replacement isServer vs calculated isServer.

isServer should only be true if the build is an SSR build, no matter how it was obtained. So you're on the right track but not in the right location.

wmertens avatar May 20 '25 14:05 wmertens