-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(server-functions): only throw SSR guard when 'window' is undefined #7606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(server-functions): only throw SSR guard when 'window' is undefined #7606
Conversation
Previously the SSR‐guard in `server-functions.ts` unconditionally threw 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 beforev - 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 QwikDev#5874
🦋 Changeset detectedLatest commit: eaff266 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
commit: |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
I don't understand - why would isServer be true in this case? |
After digging into the code, I found that Qwik’s The original guard in To resolve this, I refined the condition so we only throw errors in a real server environment (when there is no if (isServer && typeof window === 'undefined') {
throw new Error(/* ... */);
}
To validate this solution, In a quick Node REPL test(via replit), I simulated both scenarios by defining and deleting 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! |
@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. |
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:
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! |
aha, I think the problem stems from build-time-replacement
|
What is it?
Description
Previously the SSR‐guard in
server-functions.ts
unconditionally threw an error wheneverisServer
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:
This will unblock testing of actions in JSDOM environments without impacting real SSR safety
Closes #5874
Checklist
pnpm change