Skip to content

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .changeset/lovely-cycles-pay.md
Original file line number Diff line number Diff line change
@@ -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.
8 changes: 8 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/).
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
8 changes: 4 additions & 4 deletions packages/qwik-city/src/runtime/src/qwik-city-component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
_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 {
Expand Down Expand Up @@ -50,7 +50,7 @@
} 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,
Expand Down Expand Up @@ -116,7 +116,7 @@
throw new Error(`Missing Qwik URL Env Data`);
}

if (isServer) {

Check failure on line 119 in packages/qwik-city/src/runtime/src/qwik-city-component.tsx

View workflow job for this annotation

GitHub Actions / Build Other Packages

Cannot find name 'isServer'.
if (
env!.ev.originalUrl.pathname !== env!.ev.url.pathname &&
!__EXPERIMENTAL__.enableRequestRewrite
Expand Down Expand Up @@ -320,7 +320,7 @@
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;
Expand Down Expand Up @@ -624,7 +624,7 @@
}
}
const promise = run();
if (isServer) {
if (isNodeServer()) {
return promise;
} else {
return;
Expand Down
56 changes: 56 additions & 0 deletions packages/qwik-city/src/runtime/src/server-functions.spec.ts
Original file line number Diff line number Diff line change
@@ -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');
});
});
20 changes: 11 additions & 9 deletions packages/qwik-city/src/runtime/src/server-functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -218,7 +220,7 @@ export const routeLoader$: LoaderConstructor = /*#__PURE__*/ implicit$FirstArg(r
export const validatorQrl = ((
validator: QRL<(ev: RequestEvent, data: unknown) => ValueOrPromise<ValidatorReturn>>
): DataValidator => {
if (isServer) {
if (isNodeServer()) {
return {
validate: validator,
};
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -388,7 +390,7 @@ export const serverQrl = <T extends ServerFunction>(
qrl: QRL<T>,
options?: ServerConfig
): ServerQRL<T> => {
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.');
Expand All @@ -408,7 +410,7 @@ export const serverQrl = <T extends ServerFunction>(
? (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;

Expand Down
4 changes: 4 additions & 0 deletions packages/qwik-city/src/runtime/src/utils.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -72,3 +73,6 @@ export const isPromise = (value: any): value is Promise<any> => {
// 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';
18 changes: 18 additions & 0 deletions vitest.browser.config.ts
Original file line number Diff line number Diff line change
@@ -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',
},
})
);
2 changes: 1 addition & 1 deletion vitest.workspace.js
Original file line number Diff line number Diff line change
@@ -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']);
Loading