From eaff2667c927e65a765e88df18200276e1707710 Mon Sep 17 00:00:00 2001 From: Matt Williams Date: Fri, 16 May 2025 09:24:43 -0600 Subject: [PATCH 1/5] fix(server-functions): only throw SSR guard when 'window' is undefined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #5874 --- .changeset/lovely-cycles-pay.md | 22 +++++++++++++++++++ .../src/runtime/src/server-functions.ts | 3 ++- 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 .changeset/lovely-cycles-pay.md diff --git a/.changeset/lovely-cycles-pay.md b/.changeset/lovely-cycles-pay.md new file mode 100644 index 00000000000..2fa0db28a75 --- /dev/null +++ b/.changeset/lovely-cycles-pay.md @@ -0,0 +1,22 @@ +--- +'@builder.io/qwik-city': patch +--- + +FIX: Bug #5874 - SSR guard: only throw in true SSR (when window is undefined), allowing actions to run in Vitest tests + +How this change works: + +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 …; +} + +This will ensure: + +- True SSR (no window) still throws as before. +- JSDOM/Vitest tests (and browsers) have a global window, skip the throw, and return a Promise. + +This change unblocks testing of actions in JSDOM environments without impacting real SSR safety. diff --git a/packages/qwik-city/src/runtime/src/server-functions.ts b/packages/qwik-city/src/runtime/src/server-functions.ts index 193362a61ab..a11c79cbe12 100644 --- a/packages/qwik-city/src/runtime/src/server-functions.ts +++ b/packages/qwik-city/src/runtime/src/server-functions.ts @@ -88,7 +88,8 @@ export const routeActionQrl = (( }); const submit = $((input: unknown | FormData | SubmitEvent = {}) => { - if (isServer) { + // only throw in true SSR (no mock context) + if (isServer && typeof window === 'undefined') { throw new Error(`Actions can not be invoked within the server during SSR. Action.run() can only be called on the browser, for example when a user clicks a button, or submits a form.`); } From d419df2494acfa203c75e62e62fbfce507be7c55 Mon Sep 17 00:00:00 2001 From: Matt Williams Date: Mon, 4 Aug 2025 10:54:00 -0600 Subject: [PATCH 2/5] docs(server-functions): clarify SSR guard --- packages/qwik-city/src/runtime/src/server-functions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/qwik-city/src/runtime/src/server-functions.ts b/packages/qwik-city/src/runtime/src/server-functions.ts index a11c79cbe12..58893a9cabe 100644 --- a/packages/qwik-city/src/runtime/src/server-functions.ts +++ b/packages/qwik-city/src/runtime/src/server-functions.ts @@ -88,7 +88,7 @@ export const routeActionQrl = (( }); const submit = $((input: unknown | FormData | SubmitEvent = {}) => { - // only throw in true SSR (no mock context) + // only throw in true SSR when no window object is present (no mock context) if (isServer && typeof window === 'undefined') { throw new Error(`Actions can not be invoked within the server during SSR. Action.run() can only be called on the browser, for example when a user clicks a button, or submits a form.`); From a6dce877cc16ec786e38d4010399c49bc6a52819 Mon Sep 17 00:00:00 2001 From: Matt Williams Date: Mon, 4 Aug 2025 10:54:17 -0600 Subject: [PATCH 3/5] test: add routeActionQrl server window spec --- .../src/runtime/src/server-functions.spec.ts | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 packages/qwik-city/src/runtime/src/server-functions.spec.ts diff --git a/packages/qwik-city/src/runtime/src/server-functions.spec.ts b/packages/qwik-city/src/runtime/src/server-functions.spec.ts new file mode 100644 index 00000000000..fb73bb38e23 --- /dev/null +++ b/packages/qwik-city/src/runtime/src/server-functions.spec.ts @@ -0,0 +1,56 @@ +import { describe, expect, test, vi } from 'vitest'; + +// mocks for context hooks +const currentAction: any = { value: undefined }; +const location: any = { isNavigating: false }; + +vi.mock('./use-functions', () => ({ + useAction: () => currentAction, + useLocation: () => location, + useQwikCityEnv: () => ({}), +})); + +vi.mock('@builder.io/qwik', () => ({ + $: (fn: any) => fn, + _deserializeData: (d: any) => d, + _getContextElement: () => null, + _getContextEvent: () => null, + _serializeData: (d: any) => d, + _wrapProp: (v: any) => v, + implicit$FirstArg: (fn: any) => fn, + noSerialize: (v: any) => v, + useContext: () => ({}), + useStore: (init: any) => (typeof init === 'function' ? init() : init), + isServer: true, + isDev: false, +})); + +import { routeActionQrl } from './server-functions'; +import { $ } from '@builder.io/qwik'; + +// ensure window is defined even in non-JSDOM environments +if (typeof window === 'undefined') { + // @ts-ignore + globalThis.window = {} as any; +} + +describe('routeActionQrl', () => { + test('should not throw and update state when window exists on server', async () => { + const action = routeActionQrl($(async () => 'ok')); + const state = action(); + + const promise = state.submit({ foo: 'bar' }); + + expect(state.submitted).toBe(true); + expect(state.isRunning).toBe(true); + expect(location.isNavigating).toBe(true); + expect(typeof currentAction.value.resolve).toBe('function'); + + currentAction.value.resolve({ status: 200, result: 'value' }); + await expect(promise).resolves.toEqual({ status: 200, value: 'value' }); + + expect(state.isRunning).toBe(false); + expect(state.status).toBe(200); + expect(state.value).toBe('value'); + }); +}); From 476a20eb3ffdd7e05614b71ef0e344a7b9de7f96 Mon Sep 17 00:00:00 2001 From: Matt Williams Date: Mon, 4 Aug 2025 10:54:38 -0600 Subject: [PATCH 4/5] refactor: centralize node server checks --- .../src/runtime/src/qwik-city-component.tsx | 8 ++++---- .../src/runtime/src/server-functions.ts | 17 +++++++++-------- packages/qwik-city/src/runtime/src/utils.ts | 4 ++++ 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/packages/qwik-city/src/runtime/src/qwik-city-component.tsx b/packages/qwik-city/src/runtime/src/qwik-city-component.tsx index 49f801648d8..22ee1c5760b 100644 --- a/packages/qwik-city/src/runtime/src/qwik-city-component.tsx +++ b/packages/qwik-city/src/runtime/src/qwik-city-component.tsx @@ -15,7 +15,7 @@ import { _waitUntilRendered, type QRL, } from '@builder.io/qwik'; -import { isBrowser, isDev, isServer } from '@builder.io/qwik'; +import { isBrowser, isDev } from '@builder.io/qwik'; import * as qwikCity from '@qwik-city-plan'; import { CLIENT_DATA_CACHE } from './constants'; import { @@ -50,7 +50,7 @@ import type { } from './types'; import { loadClientData } from './use-endpoint'; import { useQwikCityEnv } from './use-functions'; -import { isSameOrigin, isSamePath, toUrl } from './utils'; +import { isNodeServer, isSameOrigin, isSamePath, toUrl } from './utils'; import { clientNavigate } from './client-navigate'; import { currentScrollState, @@ -309,7 +309,7 @@ export const QwikCityProvider = component$((props) => { let clientPageData: EndpointResponse | ClientPageData | undefined; let loadedRoute: LoadedRoute | null = null; let elm: unknown; - if (isServer) { + if (isNodeServer()) { // server trackUrl = new URL(navigation.dest, routeLocation.url); loadedRoute = env!.loadedRoute; @@ -610,7 +610,7 @@ export const QwikCityProvider = component$((props) => { } } const promise = run(); - if (isServer) { + if (isNodeServer()) { return promise; } else { return; diff --git a/packages/qwik-city/src/runtime/src/server-functions.ts b/packages/qwik-city/src/runtime/src/server-functions.ts index a11c79cbe12..63d0ca795e5 100644 --- a/packages/qwik-city/src/runtime/src/server-functions.ts +++ b/packages/qwik-city/src/runtime/src/server-functions.ts @@ -50,7 +50,8 @@ import type { } from './types'; import { useAction, useLocation, useQwikCityEnv } from './use-functions'; -import { isDev, isServer } from '@builder.io/qwik'; +import { isDev } from '@builder.io/qwik'; +import { isNodeServer } from './utils'; import type { FormSubmitCompletedDetail } from './form-component'; @@ -89,7 +90,7 @@ export const routeActionQrl = (( const submit = $((input: unknown | FormData | SubmitEvent = {}) => { // only throw in true SSR (no mock context) - if (isServer && typeof window === 'undefined') { + if (isNodeServer()) { throw new Error(`Actions can not be invoked within the server during SSR. Action.run() can only be called on the browser, for example when a user clicks a button, or submits a form.`); } @@ -165,7 +166,7 @@ export const globalActionQrl = (( ...rest: (CommonLoaderActionOptions | DataValidator)[] ) => { const action = routeActionQrl(actionQrl, ...(rest as any)); - if (isServer) { + if (isNodeServer()) { if (typeof globalThis._qwikActionsMap === 'undefined') { globalThis._qwikActionsMap = new Map(); } @@ -219,7 +220,7 @@ export const routeLoader$: LoaderConstructor = /*#__PURE__*/ implicit$FirstArg(r export const validatorQrl = (( validator: QRL<(ev: RequestEvent, data: unknown) => ValueOrPromise> ): DataValidator => { - if (isServer) { + if (isNodeServer()) { return { validate: validator, }; @@ -267,7 +268,7 @@ export const valibotQrl: ValibotConstructorQRL = ( 'Valibot is an experimental feature and is not enabled. Please enable the feature flag by adding `experimental: ["valibot"]` to your qwikVite plugin options.' ); } - if (isServer) { + if (isNodeServer()) { return { __brand: 'valibot', async validate(ev, inputData) { @@ -333,7 +334,7 @@ export const zodQrl: ZodConstructorQRL = ( z.ZodRawShape | z.Schema | ((z: typeof import('zod').z, ev: RequestEvent) => z.ZodRawShape) > ): ZodDataValidator => { - if (isServer) { + if (isNodeServer()) { return { __brand: 'zod', async validate(ev, inputData) { @@ -389,7 +390,7 @@ export const serverQrl = ( qrl: QRL, options?: ServerConfig ): ServerQRL => { - if (isServer) { + if (isNodeServer()) { const captured = qrl.getCaptured(); if (captured && captured.length > 0 && !_getContextElement()) { throw new Error('For security reasons, we cannot serialize QRLs that capture lexical scope.'); @@ -409,7 +410,7 @@ export const serverQrl = ( ? (args.shift() as AbortSignal) : undefined; - if (isServer) { + if (isNodeServer()) { // Running during SSR, we can call the function directly let requestEvent = globalThis.qcAsyncRequestStore?.getStore() as RequestEvent | undefined; diff --git a/packages/qwik-city/src/runtime/src/utils.ts b/packages/qwik-city/src/runtime/src/utils.ts index b09a06968fb..76b8bc631f8 100644 --- a/packages/qwik-city/src/runtime/src/utils.ts +++ b/packages/qwik-city/src/runtime/src/utils.ts @@ -1,6 +1,7 @@ import type { RouteActionValue, SimpleURL } from './types'; import { QACTION_KEY } from './constants'; +import { isServer } from '@builder.io/qwik'; /** Gets an absolute url path string (url.pathname + url.search + url.hash) */ export const toPath = (url: URL) => url.pathname + url.search + url.hash; @@ -72,3 +73,6 @@ export const isPromise = (value: any): value is Promise => { // not using "value instanceof Promise" to have zone.js support return value && typeof value.then === 'function'; }; + +/** True when running on a Node.js-like server without a DOM. */ +export const isNodeServer = () => isServer && typeof window === 'undefined'; From b3d9ccbf82e9cbf89a4c3279079f984ce287fe82 Mon Sep 17 00:00:00 2001 From: Matt Williams Date: Mon, 4 Aug 2025 10:54:56 -0600 Subject: [PATCH 5/5] feat(test): add browser vitest config --- CONTRIBUTING.md | 8 ++++++++ package.json | 1 + vitest.browser.config.ts | 18 ++++++++++++++++++ vitest.workspace.js | 2 +- 4 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 vitest.browser.config.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2bca58538b3..c33f476a2db 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -260,6 +260,14 @@ Unit tests use [vitest](https://vitest.dev) pnpm test.unit ``` +For tests that rely on browser APIs, run the DOM-focused build: + +```shell +pnpm test.unit.browser +``` + +This preset uses [`jsdom`](https://github.com/jsdom/jsdom) under the hood, so ensure it is installed. + ### E2E Tests Only E2E tests use [Playwright](https://playwright.dev/). diff --git a/package.json b/package.json index 30ac55410a1..22c8d019647 100644 --- a/package.json +++ b/package.json @@ -256,6 +256,7 @@ "test.rust": "make test", "test.rust.update": "make test-update", "test.unit": "vitest packages", + "test.unit.browser": "vitest -c vitest.browser.config.ts packages", "test.unit.debug": "vitest --inspect-brk packages", "test.vite": "playwright test starters/e2e/qwikcity --browser=chromium --config starters/playwright.config.ts", "tsc.check": "tsc --noEmit", diff --git a/vitest.browser.config.ts b/vitest.browser.config.ts new file mode 100644 index 00000000000..eed65c21e3d --- /dev/null +++ b/vitest.browser.config.ts @@ -0,0 +1,18 @@ +import { mergeConfig, defineConfig } from 'vitest/config'; +import baseConfig from './vitest.config'; + +// Browser-targeted Vitest config for tests requiring DOM APIs +export default mergeConfig( + baseConfig, + defineConfig({ + esbuild: { + platform: 'browser', + }, + resolve: { + conditions: ['browser'], + }, + test: { + environment: 'jsdom', + }, + }) +); diff --git a/vitest.workspace.js b/vitest.workspace.js index 08145815168..60c679cd943 100644 --- a/vitest.workspace.js +++ b/vitest.workspace.js @@ -1,4 +1,4 @@ import { defineWorkspace } from 'vitest/config'; // needed by the vscode vitest integration but it also speeds up vitest cli -export default defineWorkspace(['./vitest.config.ts']); +export default defineWorkspace(['./vitest.config.ts', './vitest.browser.config.ts']);