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/CONTRIBUTING.md b/CONTRIBUTING.md index 80b39b22d73..1884fb1624a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -272,6 +272,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 acb817cb2c0..eb4c3a49be4 100644 --- a/package.json +++ b/package.json @@ -254,6 +254,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/packages/qwik-city/src/runtime/src/qwik-city-component.tsx b/packages/qwik-city/src/runtime/src/qwik-city-component.tsx index 038f73c6667..6b76c09e1c6 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, @@ -320,7 +320,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; @@ -624,7 +624,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.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'); + }); +}); diff --git a/packages/qwik-city/src/runtime/src/server-functions.ts b/packages/qwik-city/src/runtime/src/server-functions.ts index 193362a61ab..4cfa5c63da0 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'; @@ -88,9 +89,10 @@ export const routeActionQrl = (( }); const submit = $((input: unknown | FormData | SubmitEvent = {}) => { - if (isServer) { + // only throw in true SSR (no mock context) + 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.`); + Action.run() can only be called on the browser, for example when a user clicks a button, or submits a form.`); } let data: unknown | FormData | SubmitEvent; let form: HTMLFormElement | undefined; @@ -164,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(); } @@ -218,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, }; @@ -266,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) { @@ -332,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) { @@ -388,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.'); @@ -408,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'; 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']);