Skip to content

Commit bc457c2

Browse files
wmertenswmertens
committed
fix(qwik-city): correctly handle q-data redirects
make sure the q-data.json stays on the URL after redirecting + mild refactoring for performance
1 parent e35ee3c commit bc457c2

File tree

9 files changed

+123
-95
lines changed

9 files changed

+123
-95
lines changed

‎.changeset/lemon-pandas-turn.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@builder.io/qwik-city': patch
3+
---
4+
5+
fix: redirecting internal q-data.json requests will keep the q-data.json suffix so that the client can still fetch the correct one

‎packages/docs/src/routes/api/qwik/api.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1774,7 +1774,7 @@
17741774
}
17751775
],
17761776
"kind": "Function",
1777-
"content": "> This API is provided as an alpha preview for developers and may change based on feedback that we receive. Do not use this API in a production environment.\n> \n\n> Warning: This API is now obsolete.\n> \n> This is no longer needed as the preloading happens automatically in qrl-class.ts. Leave this in your app for a while so it uninstalls existing service workers, but don't use it for new projects.\n> \n\n\n```typescript\nPrefetchServiceWorker: (opts: {\n base?: string;\n scope?: string;\n path?: string;\n verbose?: boolean;\n fetchBundleGraph?: boolean;\n nonce?: string;\n}) => JSXNode<'script'>\n```\n\n\n<table><thead><tr><th>\n\nParameter\n\n\n</th><th>\n\nType\n\n\n</th><th>\n\nDescription\n\n\n</th></tr></thead>\n<tbody><tr><td>\n\nopts\n\n\n</td><td>\n\n{ base?: string; scope?: string; path?: string; verbose?: boolean; fetchBundleGraph?: boolean; nonce?: string; }\n\n\n</td><td>\n\n\n</td></tr>\n</tbody></table>\n\n**Returns:**\n\nJSXNode&lt;'script'&gt;",
1777+
"content": "> This API is provided as an alpha preview for developers and may change based on feedback that we receive. Do not use this API in a production environment.\n> \n\n> Warning: This API is now obsolete.\n> \n> This is no longer needed as the preloading happens automatically in qrl-class.ts. Leave this in your app for a while so it uninstalls existing service workers, but don't use it for new projects.\n> \n\n\n```typescript\nPrefetchServiceWorker: (opts: {\n base?: string;\n scope?: string;\n path?: string;\n verbose?: boolean;\n fetchBundleGraph?: boolean;\n nonce?: string;\n}) => JSXNode<'script'>\n```\n\n\n<table><thead><tr><th>\n\nParameter\n\n\n</th><th>\n\nType\n\n\n</th><th>\n\nDescription\n\n\n</th></tr></thead>\n<tbody><tr><td>\n\nopts\n\n\n</td><td>\n\n{ base?: string; scope?: string; path?: string; verbose?: boolean; fetchBundleGraph?: boolean; nonce?: string; }\n\n\n</td><td>\n\n\n</td></tr>\n</tbody></table>\n\n**Returns:**\n\n[JSXNode](#jsxnode)<!-- -->&lt;'script'&gt;",
17781778
"editUrl": "https://github.com/QwikDev/qwik/tree/main/packages/qwik/src/core/components/prefetch.ts",
17791779
"mdFile": "qwik.prefetchserviceworker.md"
17801780
},

‎packages/docs/src/routes/api/qwik/index.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3667,7 +3667,7 @@ opts
36673667
36683668
**Returns:**
36693669
3670-
JSXNode&lt;'script'&gt;
3670+
[JSXNode](#jsxnode)&lt;'script'&gt;
36713671
36723672
[Edit this section](https://github.com/QwikDev/qwik/tree/main/packages/qwik/src/core/components/prefetch.ts)
36733673

‎packages/qwik-city/src/buildtime/vite/dev-server.ts‎

Lines changed: 35 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -9,23 +9,19 @@ import {
99
resolveRequestHandlers,
1010
} from '../../middleware/request-handler/resolve-request-handlers';
1111
import { getQwikCityServerData } from '../../middleware/request-handler/response-page';
12-
import {
13-
QDATA_JSON,
14-
getRouteMatchPathname,
15-
runQwikCity,
16-
} from '../../middleware/request-handler/user-response';
12+
import { getRouteMatchPathname, runQwikCity } from '../../middleware/request-handler/user-response';
1713
import { matchRoute } from '../../runtime/src/route-matcher';
1814
import { getMenuLoader } from '../../runtime/src/routing';
1915
import type {
2016
ActionInternal,
21-
RebuildRouteInfoInternal,
2217
ContentMenu,
2318
LoadedRoute,
2419
LoaderInternal,
2520
MenuData,
2621
MenuModule,
2722
MenuModuleLoader,
2823
PathParams,
24+
RebuildRouteInfoInternal,
2925
RequestEvent,
3026
RouteModule,
3127
} from '../../runtime/src/types';
@@ -126,16 +122,18 @@ export function ssrDevMiddleware(ctx: BuildContext, server: ViteDevServer) {
126122
] satisfies LoadedRoute;
127123
return { serverPlugins, loadedRoute };
128124
};
129-
const resolveRoute = (routeModulePaths: WeakMap<RouteModule<unknown>, string>, url: URL) => {
130-
const matchPathname = getRouteMatchPathname(url.pathname, ctx.opts.trailingSlash);
131-
routePs[matchPathname] ||= _resolveRoute(routeModulePaths, matchPathname).finally(() => {
132-
delete routePs[matchPathname];
125+
const resolveRoute = (
126+
routeModulePaths: WeakMap<RouteModule<unknown>, string>,
127+
pathname: string
128+
) => {
129+
routePs[pathname] ||= _resolveRoute(routeModulePaths, pathname).finally(() => {
130+
delete routePs[pathname];
133131
});
134-
return routePs[matchPathname];
132+
return routePs[pathname];
135133
};
136134

137135
// Preload the modules needed to handle /, so that they load faster on first request.
138-
resolveRoute(new WeakMap(), new URL('/', 'http://localhost')).catch((e: unknown) => {
136+
resolveRoute(new WeakMap(), '/').catch((e: unknown) => {
139137
if (e instanceof Error) {
140138
server.ssrFixStacktrace(e);
141139
formatError(e);
@@ -150,32 +148,34 @@ export function ssrDevMiddleware(ctx: BuildContext, server: ViteDevServer) {
150148
next();
151149
return;
152150
}
151+
const { pathname, isInternal } = getRouteMatchPathname(url.pathname, ctx.opts.trailingSlash);
152+
153+
if (!isInternal) {
154+
// Normally, entries are served statically, so in dev mode we need to handle them here.
155+
const matchRouteName = url.pathname.slice(1);
156+
const entry = ctx.entries.find((e) => e.routeName === matchRouteName);
157+
if (entry) {
158+
const entryContents = await server.transformRequest(
159+
`/@fs${entry.filePath.startsWith('/') ? '' : '/'}${entry.filePath}`
160+
);
153161

154-
// Normally, entries are served statically, so in dev mode we need to handle them here.
155-
const matchRouteName = url.pathname.slice(1);
156-
const entry = ctx.entries.find((e) => e.routeName === matchRouteName);
157-
if (entry) {
158-
const entryContents = await server.transformRequest(
159-
`/@fs${entry.filePath.startsWith('/') ? '' : '/'}${entry.filePath}`
160-
);
161-
162-
if (entryContents) {
163-
res.setHeader('Content-Type', 'text/javascript');
164-
res.end(entryContents.code);
165-
} else {
166-
next();
162+
if (entryContents) {
163+
res.setHeader('Content-Type', 'text/javascript');
164+
res.end(entryContents.code);
165+
} else {
166+
next();
167+
}
168+
return;
167169
}
168-
return;
169170
}
170171

171172
const routeModulePaths = new WeakMap<RouteModule, string>();
172173
try {
173-
const { serverPlugins, loadedRoute } = await resolveRoute(routeModulePaths, url);
174+
const { serverPlugins, loadedRoute } = await resolveRoute(routeModulePaths, pathname);
174175

175176
const renderFn = async (requestEv: RequestEvent) => {
176177
// routeResult && requestEv.sharedMap.set('@routeName', routeResult.route.pathname);
177-
const isPageDataReq = requestEv.pathname.endsWith(QDATA_JSON);
178-
if (!isPageDataReq) {
178+
if (!isInternal) {
179179
const serverData = getQwikCityServerData(requestEv);
180180

181181
res.statusCode = requestEv.status();
@@ -217,7 +217,8 @@ export function ssrDevMiddleware(ctx: BuildContext, server: ViteDevServer) {
217217
loadedRoute,
218218
req.method ?? 'GET',
219219
false,
220-
renderFn
220+
renderFn,
221+
isInternal
221222
);
222223

223224
if (requestHandlers.length > 0) {
@@ -229,13 +230,15 @@ export function ssrDevMiddleware(ctx: BuildContext, server: ViteDevServer) {
229230
const qwikSerializer = { _deserializeData, _serializeData, _verifySerializable };
230231

231232
const rebuildRouteInfo: RebuildRouteInfoInternal = async (url: URL) => {
232-
const { serverPlugins, loadedRoute } = await resolveRoute(routeModulePaths, url);
233+
const { pathname } = getRouteMatchPathname(url.pathname, ctx.opts.trailingSlash);
234+
const { serverPlugins, loadedRoute } = await resolveRoute(routeModulePaths, pathname);
233235
const requestHandlers = resolveRequestHandlers(
234236
serverPlugins,
235237
loadedRoute,
236238
req.method ?? 'GET',
237239
false,
238-
renderFn
240+
renderFn,
241+
isInternal
239242
);
240243

241244
return {

‎packages/qwik-city/src/middleware/request-handler/request-event.ts‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import type {
3030
ServerRequestEvent,
3131
ServerRequestMode,
3232
} from './types';
33-
import { IsQData, QDATA_JSON, QDATA_JSON_LEN } from './user-response';
33+
import { IsQData, getRouteMatchPathname } from './user-response';
3434

3535
const RequestEvLoaders = Symbol('RequestEvLoaders');
3636
const RequestEvMode = Symbol('RequestEvMode');
@@ -58,11 +58,11 @@ export function createRequestEvent(
5858
const cookie = new Cookie(request.headers.get('cookie'));
5959
const headers = new Headers();
6060
const url = new URL(request.url);
61-
if (url.pathname.endsWith(QDATA_JSON)) {
62-
url.pathname = url.pathname.slice(0, -QDATA_JSON_LEN);
63-
if (trailingSlash && !url.pathname.endsWith('/')) {
64-
url.pathname += '/';
65-
}
61+
const { pathname, isInternal } = getRouteMatchPathname(url.pathname, trailingSlash);
62+
if (isInternal) {
63+
// For the middleware callbacks we pretend it's a regular request
64+
url.pathname = pathname;
65+
// But we set this flag so that they can act differently
6666
sharedMap.set(IsQData, true);
6767
}
6868

@@ -77,10 +77,7 @@ export function createRequestEvent(
7777

7878
while (routeModuleIndex < requestHandlers.length) {
7979
const moduleRequestHandler = requestHandlers[routeModuleIndex];
80-
const asyncStore = globalThis.qcAsyncRequestStore;
81-
const result = asyncStore?.run
82-
? asyncStore.run(requestEv, moduleRequestHandler, requestEv)
83-
: moduleRequestHandler(requestEv);
80+
const result = moduleRequestHandler(requestEv);
8481
if (isPromise(result)) {
8582
await result;
8683
}
@@ -236,17 +233,19 @@ export function createRequestEvent(
236233
check();
237234
status = statusCode;
238235
if (url) {
239-
const fixedURL = url.replace(/([^:])\/{2,}/g, '$1/');
240-
if (url !== fixedURL) {
236+
if (/([^:])\/{2,}/.test(url)) {
237+
const fixedURL = url.replace(/([^:])\/{2,}/g, '$1/');
241238
console.warn(`Redirect URL ${url} is invalid, fixing to ${fixedURL}`);
239+
url = fixedURL;
242240
}
243-
headers.set('Location', fixedURL);
241+
headers.set('Location', url);
244242
}
245243
headers.delete('Cache-Control');
246244
if (statusCode > 301) {
247245
headers.set('Cache-Control', 'no-store');
248246
}
249-
exit();
247+
248+
routeModuleIndex = ABORT_INDEX;
250249
return new RedirectMessage();
251250
},
252251

‎packages/qwik-city/src/middleware/request-handler/request-handler.ts‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,27 +16,32 @@ export async function requestHandler<T = unknown>(
1616
qwikSerializer: QwikSerializer
1717
): Promise<QwikCityRun<T> | null> {
1818
const { render, qwikCityPlan, checkOrigin } = opts;
19-
const pathname = serverRequestEv.url.pathname;
20-
const matchPathname = getRouteMatchPathname(pathname, qwikCityPlan.trailingSlash);
19+
const { pathname, isInternal } = getRouteMatchPathname(
20+
serverRequestEv.url.pathname,
21+
qwikCityPlan.trailingSlash
22+
);
2123
const routeAndHandlers = await loadRequestHandlers(
2224
qwikCityPlan,
23-
matchPathname,
25+
pathname,
2426
serverRequestEv.request.method,
2527
checkOrigin ?? true,
26-
render
28+
render,
29+
isInternal
2730
);
2831

2932
if (routeAndHandlers) {
3033
const [route, requestHandlers] = routeAndHandlers;
3134

3235
const rebuildRouteInfo: RebuildRouteInfoInternal = async (url: URL) => {
33-
const matchPathname = getRouteMatchPathname(url.pathname, qwikCityPlan.trailingSlash);
36+
// once internal, always internal, don't override
37+
const { pathname } = getRouteMatchPathname(url.pathname, qwikCityPlan.trailingSlash);
3438
const routeAndHandlers = await loadRequestHandlers(
3539
qwikCityPlan,
36-
matchPathname,
40+
pathname,
3741
serverRequestEv.request.method,
3842
checkOrigin ?? true,
39-
render
43+
render,
44+
isInternal
4045
);
4146

4247
if (routeAndHandlers) {
@@ -65,16 +70,18 @@ async function loadRequestHandlers(
6570
pathname: string,
6671
method: string,
6772
checkOrigin: boolean | 'lax-proto',
68-
renderFn: Render
73+
renderFn: Render,
74+
isInternal: boolean
6975
) {
7076
const { routes, serverPlugins, menus, cacheModules } = qwikCityPlan;
71-
const route = await loadRoute(routes, menus, cacheModules, pathname);
77+
const route = await loadRoute(routes, menus, cacheModules, pathname, isInternal);
7278
const requestHandlers = resolveRequestHandlers(
7379
serverPlugins,
7480
route,
7581
method,
7682
checkOrigin,
77-
renderQwikMiddleware(renderFn)
83+
renderQwikMiddleware(renderFn),
84+
isInternal
7885
);
7986
if (requestHandlers.length > 0) {
8087
return [route, requestHandlers] as const;

‎packages/qwik-city/src/middleware/request-handler/resolve-request-handlers.ts‎

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,20 @@ import { IsQData, QDATA_JSON } from './user-response';
3535
// Import separately to avoid duplicate imports in the vite dev server
3636
import { RedirectMessage, ServerError } from '@builder.io/qwik-city/middleware/request-handler';
3737

38+
/**
39+
* This generates the handlers that will be run. They run in the order they are defined. If one
40+
* calls `.exit()`, the rest of the handlers will be skipped.
41+
*
42+
* By awaiting `.next()`, handlers can wait for all subsequent handlers to complete before
43+
* continuing.
44+
*/
3845
export const resolveRequestHandlers = (
3946
serverPlugins: RouteModule[] | undefined,
4047
route: LoadedRoute | null,
4148
method: string,
4249
checkOrigin: boolean | 'lax-proto',
43-
renderHandler: RequestHandler
50+
renderHandler: RequestHandler,
51+
isInternal: boolean
4452
) => {
4553
const routeLoaders: LoaderInternal[] = [];
4654
const routeActions: ActionInternal[] = [];
@@ -49,7 +57,13 @@ export const resolveRequestHandlers = (
4957

5058
const isPageRoute = !!(route && isLastModulePageRoute(route[2]));
5159

60+
// Always handle QData redirects (server plugins might redirect)
61+
if (isInternal) {
62+
requestHandlers.push(handleQDataRedirect);
63+
}
64+
5265
if (serverPlugins) {
66+
// Serverplugins run even if no route is matched
5367
_resolveRequestHandlers(
5468
routeLoaders,
5569
routeActions,
@@ -73,16 +87,20 @@ export const resolveRequestHandlers = (
7387
}
7488
}
7589
if (isPageRoute) {
76-
// server$
90+
// `server$()` can only be called from existing page routes
7791
if (method === 'POST' || method === 'GET') {
78-
requestHandlers.push(pureServerFunction);
92+
requestHandlers.push(runServerFunction);
7993
}
8094

95+
// Note that we don't care about trailing slash on `server$()` calls
8196
requestHandlers.push(fixTrailingSlash);
82-
requestHandlers.push(renderQData);
97+
98+
// If this is a QData request, we short-circuit after running all the loaders/middleware
99+
if (isInternal) {
100+
requestHandlers.push(renderQData);
101+
}
83102
}
84103
const routeModules = route[2];
85-
requestHandlers.push(handleRedirect);
86104
_resolveRequestHandlers(
87105
routeLoaders,
88106
routeActions,
@@ -304,7 +322,7 @@ function isAsyncIterator(obj: unknown): obj is AsyncIterable<unknown> {
304322
return obj ? typeof obj === 'object' && Symbol.asyncIterator in obj : false;
305323
}
306324

307-
async function pureServerFunction(ev: RequestEvent) {
325+
async function runServerFunction(ev: RequestEvent) {
308326
const fn = ev.query.get(QFN_KEY);
309327
if (
310328
fn &&
@@ -521,12 +539,8 @@ export function renderQwikMiddleware(render: Render) {
521539
};
522540
}
523541

524-
export async function handleRedirect(requestEv: RequestEvent) {
525-
const isPageDataReq = requestEv.sharedMap.has(IsQData);
526-
if (!isPageDataReq) {
527-
return;
528-
}
529-
542+
/** Restore q-data.json on redirect */
543+
async function handleQDataRedirect(requestEv: RequestEvent) {
530544
try {
531545
await requestEv.next();
532546
} catch (err) {
@@ -555,11 +569,7 @@ export async function handleRedirect(requestEv: RequestEvent) {
555569
}
556570
}
557571

558-
export async function renderQData(requestEv: RequestEvent) {
559-
const isPageDataReq = requestEv.sharedMap.has(IsQData);
560-
if (!isPageDataReq) {
561-
return;
562-
}
572+
async function renderQData(requestEv: RequestEvent) {
563573
await requestEv.next();
564574

565575
if (requestEv.headersSent || requestEv.exited) {
@@ -594,11 +604,13 @@ export async function renderQData(requestEv: RequestEvent) {
594604

595605
function makeQDataPath(href: string) {
596606
if (href.startsWith('/')) {
597-
const append = QDATA_JSON;
598-
const url = new URL(href, 'http://localhost');
607+
if (!href.includes(QDATA_JSON)) {
608+
const url = new URL(href, 'http://localhost');
599609

600-
const pathname = url.pathname.endsWith('/') ? url.pathname.slice(0, -1) : url.pathname;
601-
return pathname + (append.startsWith('/') ? '' : '/') + append + url.search;
610+
const pathname = url.pathname.endsWith('/') ? url.pathname.slice(0, -1) : url.pathname;
611+
return pathname + QDATA_JSON + url.search;
612+
}
613+
return href;
602614
} else {
603615
return undefined;
604616
}

0 commit comments

Comments
 (0)