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

SSG: support wildcards in static paths #666

Merged
merged 15 commits into from
May 5, 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
22 changes: 22 additions & 0 deletions e2e/fixtures/ssg-wildcard/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"name": "waku-example",
"version": "0.1.0",
"type": "module",
"private": true,
"scripts": {
"dev": "waku dev",
"build": "waku build",
"start": "waku start"
},
"dependencies": {
"react": "19.0.0-beta-4508873393-20240430",
"react-dom": "19.0.0-beta-4508873393-20240430",
"react-server-dom-webpack": "19.0.0-beta-4508873393-20240430",
"waku": "workspace:*"
},
"devDependencies": {
"@types/react": "18.3.1",
"@types/react-dom": "18.3.0",
"typescript": "5.4.4"
}
}
15 changes: 15 additions & 0 deletions e2e/fixtures/ssg-wildcard/src/main.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { StrictMode } from 'react';
import { createRoot, hydrateRoot } from 'react-dom/client';
import { Router } from 'waku/router/client';

const rootElement = (
<StrictMode>
<Router />
</StrictMode>
);

if (document.body.dataset.hydrate) {
hydrateRoot(document.body, rootElement);
} else {
createRoot(document.body).render(rootElement);
}
14 changes: 14 additions & 0 deletions e2e/fixtures/ssg-wildcard/src/pages/[...wildcard].tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const Page = ({ wildcard }: { wildcard: string[] }) => (
<div>
<h1>/{wildcard.join('/')}</h1>
</div>
);

export const getConfig = async () => {
return {
render: 'static',
staticPaths: [[], 'foo', ['bar', 'baz']],
};
};
Comment on lines +7 to +12
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't type checked.
I wonder whether we should make ssg-wildcard with createPages API instead of fs-router.

We can say that types are covered with unit tests, but still fixtures should be as basic as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it is not type-covered in createPages either:

// TODO: This fails at runtime, but not at type level.
// \@ts-expect-error: static paths do not match the slug pattern
createPage({
render: 'static',
path: '/test/[a]/[b]',
staticPaths: ['c'],
component: () => 'Hello',
});

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. I hope we can fix it.

My question still holds: Should we make fixtures use low-level createPages instead of fs-router? (Unless we specifically tests fs-router.)
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. My gut feeling would be to use integration tests to cover as much ground as possible, but in this case createPages is a common entry point (and even my preferred one). Also fs-router.ts is a good target for a unit test.
But this then should also apply to partial-build and ssg-performance. And most other tests don't use createPages but defineRouter. Maybe another PR where we look at the e2e setup holistically, including the typing hiccups that we encountered?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Sounds all good!


export default Page;
10 changes: 10 additions & 0 deletions e2e/fixtures/ssg-wildcard/src/pages/_layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import type { ReactNode } from 'react';

const Layout = ({ children }: { children: ReactNode }) => (
<div>
<title>Waku</title>
{children}
</div>
);

export default Layout;
16 changes: 16 additions & 0 deletions e2e/fixtures/ssg-wildcard/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"compilerOptions": {
"composite": true,
"strict": true,
"target": "esnext",
"downlevelIteration": true,
"esModuleInterop": true,
"module": "esnext",
"moduleResolution": "bundler",
"skipLibCheck": true,
"noUncheckedIndexedAccess": true,
"exactOptionalPropertyTypes": true,
"types": ["react/experimental"],
"jsx": "react-jsx"
}
}
74 changes: 74 additions & 0 deletions e2e/ssg-wildcard.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { debugChildProcess, getFreePort, terminate, test } from './utils.js';
import { fileURLToPath } from 'node:url';
import { cp, mkdtemp } from 'node:fs/promises';
import { exec, execSync } from 'node:child_process';
import { expect } from '@playwright/test';
import waitPort from 'wait-port';
import { join } from 'node:path';
import { tmpdir } from 'node:os';
import { createRequire } from 'node:module';

let standaloneDir: string;
const exampleDir = fileURLToPath(
new URL('./fixtures/ssg-wildcard', import.meta.url),
);
const wakuDir = fileURLToPath(new URL('../packages/waku', import.meta.url));
const { version } = createRequire(import.meta.url)(
join(wakuDir, 'package.json'),
);

test.describe('ssg wildcard', async () => {
test.beforeEach(async () => {
// GitHub Action on Windows doesn't support mkdtemp on global temp dir,
// Which will cause files in `src` folder to be empty.
// I don't know why
const tmpDir = process.env.TEMP_DIR ? process.env.TEMP_DIR : tmpdir();
Comment on lines +22 to +25
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, does that mean TEMP_DIR is a workaround for Windows?

@himself65 thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but I think we could just make things under .cache folder? I forget why I prefer to use tmpdir here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this comment is copied from e2e/07_router_standalone.spec.ts.
Okay, please tackle it separately from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, copy from my code 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, copy from my code 😆

I like to call it "proudly found elsewhere".

standaloneDir = await mkdtemp(join(tmpDir, 'waku-ssg-wildcard-'));
await cp(exampleDir, standaloneDir, {
filter: (src) => {
return !src.includes('node_modules') && !src.includes('dist');
},
recursive: true,
});
execSync(`pnpm pack --pack-destination ${standaloneDir}`, {
cwd: wakuDir,
stdio: 'inherit',
});
const name = `waku-${version}.tgz`;
execSync(`npm install ${join(standaloneDir, name)}`, {
cwd: standaloneDir,
stdio: 'inherit',
});
});

test(`works`, async ({ page }) => {
execSync(
`node ${join(standaloneDir, './node_modules/waku/dist/cli.js')} build`,
{
cwd: standaloneDir,
stdio: 'inherit',
},
);
const port = await getFreePort();
const cp = exec(
`node ${join(standaloneDir, './node_modules/waku/dist/cli.js')} start --port ${port}`,
{ cwd: standaloneDir },
);
debugChildProcess(cp, fileURLToPath(import.meta.url), [
/ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time/,
]);

await waitPort({ port });

await page.goto(`http://localhost:${port}`);
await expect(page.getByRole('heading', { name: '/' })).toBeVisible();

await page.goto(`http://localhost:${port}/foo`);
await expect(page.getByRole('heading', { name: '/foo' })).toBeVisible();

await page.goto(`http://localhost:${port}/bar/baz`);
await expect(page.getByRole('heading', { name: '/bar/baz' })).toBeVisible();

await terminate(cp.pid!);
});
});
32 changes: 20 additions & 12 deletions packages/waku/src/router/create-pages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export type CreatePage = <
}
| {
render: 'static';
path: PathWithSlug<Path, SlugKey>;
path: PathWithWildcard<Path, SlugKey, WildSlugKey>;
staticPaths: string[] | string[][];
component: FunctionComponent<RouteProps & Record<SlugKey, string>>;
}
Expand Down Expand Up @@ -170,27 +170,35 @@ export function createPages(
staticPathSet.add([page.path, pathSpec]);
const id = joinPath(page.path, 'page').replace(/^\//, '');
registerStaticComponent(id, page.component);
} else if (page.render === 'static' && numSlugs > 0 && numWildcards === 0) {
} else if (page.render === 'static' && numSlugs > 0) {
const staticPaths = (
page as {
staticPaths: string[] | string[][];
}
).staticPaths.map((item) => (Array.isArray(item) ? item : [item]));
for (const staticPath of staticPaths) {
if (staticPath.length !== numSlugs) {
if (staticPath.length !== numSlugs && numWildcards === 0) {
throw new Error('staticPaths does not match with slug pattern');
}
const mapping: Record<string, string> = {};
const mapping: Record<string, string | string[]> = {};
let slugIndex = 0;
const pathItems = pathSpec.map(({ type, name }) => {
if (type !== 'literal') {
const actualName = staticPath[slugIndex++]!;
if (name) {
mapping[name] = actualName;
}
return actualName;
const pathItems = [] as string[];
pathSpec.forEach(({ type, name }) => {
switch (type) {
case 'literal':
pathItems.push(name!);
break;
case 'wildcard':
mapping[name!] = staticPath.slice(slugIndex);
staticPath.slice(slugIndex++).forEach((slug) => {
pathItems.push(slug);
});
break;
case 'group':
pathItems.push(staticPath[slugIndex++]!);
mapping[name!] = pathItems[pathItems.length - 1]!;
break;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, now you read the create-page.ts code, do you have any idea to improve it or completely rewrite it?
I wonder how readable the code is. It's unlikely that the code is easy to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did find my way in reasonable time, so it can't be that bad 😄
I fully agree with the comment on line 29, but there is no unit test framework. I suppose we would use vitest?

// FIXME we should add unit tests for some functions and type utils.

Happy to take a stab at this, in a separate PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be more than helpful!
Yes, let's add vitest. A separate PR would be perfect.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, ts-expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There we go: #667

I created a pure setup-PR first, since i'm also working on another PR where vitest would help.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you add some tests for this?
Maybe we should have a separate PR to add tests without this PR change first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was not too easy to find a way to test it. But i think this gives good coverage to do a confident refactor in a later step: #692

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the unit tests immediately revealed a regression! 😅

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nice! Yeah, because I was very unsure that I could review the code.

}
return name;
});
staticPathSet.add([
page.path,
Expand Down
45 changes: 38 additions & 7 deletions packages/waku/tests/create-pages.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -515,19 +515,50 @@ describe('createPages', () => {
expect(TestPage).toHaveBeenCalledWith({ a: 'w', b: 'x' }, undefined);
});

it('fails when trying to create a static page with wildcards', async () => {
it('creates a static page with wildcards', async () => {
const TestPage = vi.fn();
createPages(async ({ createPage }) => {
// @ts-expect-error: this already fails at type level, but we also want to test runtime
createPage({
render: 'static',
path: '/test/[...path]',
component: () => null,
staticPaths: [['a', 'b']],
component: TestPage,
});
});
const { getPathConfig } = injectedFunctions();
await expect(getPathConfig).rejects.toThrowError(
`Invalid page configuration`,
);
const { getPathConfig, getComponent } = injectedFunctions();
expect(await getPathConfig!()).toEqual([
{
data: undefined,
isStatic: true,
noSsr: false,
path: [
{
name: 'test',
type: 'literal',
},
{
name: 'a',
type: 'literal',
},
{
name: 'b',
type: 'literal',
},
],
pattern: '^/test/(.*)$',
},
]);
const setShouldSkip = vi.fn();
const WrappedComponent = await getComponent('test/a/b/page', {
unstable_setShouldSkip: setShouldSkip,
unstable_buildConfig: undefined,
});
assert(WrappedComponent);
expect(setShouldSkip).toHaveBeenCalledTimes(1);
expect(setShouldSkip).toHaveBeenCalledWith([]);
renderToString(createElement(WrappedComponent as any));
expect(TestPage).toHaveBeenCalledTimes(1);
expect(TestPage).toHaveBeenCalledWith({ path: ['a', 'b'] }, undefined);
});

it('creates a dynamic page with wildcards', async () => {
Expand Down
25 changes: 25 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tsconfig.e2e.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
{
"path": "./e2e/fixtures/ssg-performance/tsconfig.json"
},
{
"path": "./e2e/fixtures/ssg-wildcard/tsconfig.json"
},
{
"path": "./e2e/fixtures/partial-build/tsconfig.json"
}
Expand Down
Loading