Skip to content
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

fix(qwik-city): strip internal parameters for compare #6694

Merged
merged 4 commits into from
Jul 18, 2024
Merged
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
5 changes: 5 additions & 0 deletions .changeset/lucky-dolphins-grab.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@builder.io/qwik-city': patch
---

During dev mode, qwik-city will no longer serve files from `dist/`, which are very likely to be stale/incorrect. Furthermore, query parameters are taken into account when serving files (like production servers would do).
5 changes: 5 additions & 0 deletions .changeset/rich-hairs-worry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@builder.io/qwik-city': patch
---

strip internal search parameters in canonical URLs
2 changes: 1 addition & 1 deletion packages/qwik-city/buildtime/vite/dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ export function staticDistMiddleware({ config }: ViteDevServer) {
return;
}

const relPath = url.pathname.slice(1);
const relPath = `${url.pathname.slice(1)}${url.search}`;

const ext = getExtension(relPath);
const contentType = STATIC_CONTENT_TYPES[ext];
Expand Down
19 changes: 11 additions & 8 deletions packages/qwik-city/buildtime/vite/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function qwikCityPlugin(userOpts?: QwikCityVitePluginOptions): any {

ctx = createBuildContext(rootDir!, config.base, userOpts, target);

ctx.isDevServer = config.command === 'serve';
ctx.isDevServer = config.command === 'serve' && config.mode !== 'production';
ctx.isDevServerClientOnly = ctx.isDevServer && config.mode !== 'ssr';

await validatePlugin(ctx.opts);
Expand All @@ -105,14 +105,17 @@ function qwikCityPlugin(userOpts?: QwikCityVitePluginOptions): any {

configureServer(server) {
return () => {
// handles static files physically found in the dist directory
server.middlewares.use(staticDistMiddleware(server));
if (ctx) {
// qwik city middleware injected BEFORE vite internal middlewares
// and BEFORE @builder.io/qwik/optimizer/vite middlewares
// handles only known user defined routes
server.middlewares.use(ssrDevMiddleware(ctx, server));
if (!ctx) {
throw new Error('configureServer: Missing ctx from configResolved');
}
if (!ctx.isDevServer) {
// preview server: serve static files from the dist directory
server.middlewares.use(staticDistMiddleware(server));
}
// qwik city middleware injected BEFORE vite internal middlewares
// and BEFORE @builder.io/qwik/optimizer/vite middlewares
// handles only known user defined routes
server.middlewares.use(ssrDevMiddleware(ctx, server));
};
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,15 @@ const _resolveRequestHandlers = (
}

if (collectActions) {
const loaders = Object.values(routeModule).filter((e) =>
checkBrand(e, 'server_loader')
) as LoaderInternal[];
routeLoaders.push(...loaders);

const actions = Object.values(routeModule).filter((e) =>
checkBrand(e, 'server_action')
) as ActionInternal[];
routeActions.push(...actions);
for (const module of Object.values(routeModule)) {
if (typeof module === 'function') {
if (module.__brand === 'server_loader') {
routeLoaders.push(module as LoaderInternal);
} else if (module.__brand === 'server_action') {
routeActions.push(module as ActionInternal);
}
}
}
}
}
};
Expand Down Expand Up @@ -407,7 +407,9 @@ export function getPathname(url: URL, trailingSlash: boolean | undefined) {
url.pathname = url.pathname.slice(0, -1);
}
}
return url.toString().substring(url.origin.length);
// strip internal search params
const search = url.search.slice(1).replaceAll(/&?q(action|data|func)=[^&]+/g, '');
wmertens marked this conversation as resolved.
Show resolved Hide resolved
return `${url.pathname}${search ? `?${search}` : ''}${url.hash}`;
}

export const encoder = /*#__PURE__*/ new TextEncoder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,14 @@ describe('resolve-request-handler', () => {
'/path?foo=bar#hash'
);
});

it('should remove internal search params', () => {
expect(getPathname(new URL('http://server/path?qaction=123&qdata=data'), true)).toBe(
'/path/'
);
expect(getPathname(new URL('http://server/path?foo=1&qfunc=f&bar=2'), false)).toBe(
'/path?foo=1&bar=2'
);
});
});
});
1 change: 1 addition & 0 deletions packages/qwik-city/runtime/src/qwik-city-component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ export const QwikCityProvider = component$<QwikCityProps>((props) => {
const newHref = pageData.href;
const newURL = new URL(newHref, trackUrl);
if (!isSamePath(newURL, trackUrl)) {
// Change our path to the canonical path in the response.
trackUrl = newURL;
loadRoutePromise = loadRoute(
qwikCity.routes,
Expand Down
4 changes: 2 additions & 2 deletions scripts/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,15 @@ export async function build(config: BuildConfig) {
await buildWasmBinding(config);
}

if (config.tsc) {
if (config.tsc || (!config.dev && config.qwikcity)) {
await tscQwikCity(config);
}

if (config.qwikcity) {
await buildQwikCity(config);
}

if (config.api || (config.tsc && config.qwikcity)) {
if (config.api || ((!config.dev || config.tsc) && config.qwikcity)) {
await apiExtractorQwikCity(config);
}

Expand Down
18 changes: 18 additions & 0 deletions starters/apps/qwikcity-test/src/routes/issue6660/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { routeAction$, Form } from "@builder.io/qwik-city";
import { component$ } from "@builder.io/qwik";

export const useAction = routeAction$(() => ({ ok: true }));

export default component$(() => {
const action = useAction();

return (
<Form action={action}>
<button name="test" value="test">
Submit
</button>

{action.value?.ok && <span id="status">Submitted</span>}
</Form>
);
});
29 changes: 29 additions & 0 deletions starters/e2e/qwikcity/nav.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,35 @@ test.describe("actions", () => {
await expect(count).toHaveText("Count: 1");
});
});

test("issue 6660 internal params should not trigger navigation", async ({
page,
}) => {
await page.goto("/qwikcity-test/issue6660/");
await expect(page.locator("#status")).toBeHidden();

{
const startUrl = page.url();

await page.getByText("Submit").click();
await page.waitForSelector("#status");

expect(page.url()).toBe(startUrl);
}

await page.goto("/qwikcity-test/issue6660/?var=1&hello");
await expect(page.locator("#status")).toBeHidden();

{
const startUrl = page.url();
expect(startUrl).toContain("var=1&hello");

await page.getByText("Submit").click();
await page.waitForSelector("#status");

expect(page.url()).toBe(startUrl);
}
});
}

function tests() {
Expand Down
Loading