SSG: support wildcards in static paths #666

pmelab · 2024-04-23T08:06:56Z

Allow wildcards in static paths. The main use case would be scenarios where waku is used as a static site generator and paths are controlled by a headless CMS.

vercel · 2024-04-23T08:07:00Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
waku	✅ Ready (Inspect)	Visit Preview	May 5, 2024 9:55am

codesandbox-ci · 2024-04-23T08:07:18Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

dai-shi · 2024-04-23T08:19:41Z

Wow, thanks for working on it. Let me do a quick first review.

dai-shi

https://github.com/dai-shi/waku/pull/666/files#diff-aa5fd10e2174215f8433b37357a7b91805c9a9d4d00c232fd563ce9f1119651dR69-R92

I guess types has to be updated, no?

dai-shi · 2024-04-23T08:21:58Z

e2e/25_static-wildcard.spec.ts

+
+let standaloneDir: string;
+const exampleDir = fileURLToPath(
+  new URL('../examples/25_static-wildcard', import.meta.url),


Can you move it to fixtures instead of examples?

dai-shi · 2024-04-23T08:22:28Z

e2e/25_static-wildcard.spec.ts

+    // 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();
+    standaloneDir = await mkdtemp(join(tmpDir, 'waku-07-router-'));


07-router sounds strange here.

Copy & paste 🙈

dai-shi · 2024-04-23T08:25:23Z

packages/waku/src/router/create-pages.ts

+              break;
+            case 'group':
+              pathItems.push(staticPath[slugIndex++]!);
+              break;


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.

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?

waku/packages/waku/src/router/create-pages.ts

Line 29 in 098b6b3

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

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

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

And, ts-expect.

There we go: #667

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

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

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

So the unit tests immediately revealed a regression! 😅

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

dai-shi

first review

thanks for working on it. this seems like a good improvement.

dai-shi · 2024-05-02T01:10:55Z

e2e/ssg-wildcard.spec.ts

+    // 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();


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

@himself65 thoughts?

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

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

yes, copy from my code 😆

yes, copy from my code 😆

I like to call it "proudly found elsewhere".

package.json

dai-shi · 2024-05-02T01:14:16Z

packages/waku/src/router/create-pages.ts

+              break;
+            case 'group':
+              pathItems.push(staticPath[slugIndex++]!);
+              break;


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

# Conflicts: # pnpm-lock.yaml # tsconfig.e2e.json

dai-shi

second review

dai-shi · 2024-05-03T23:34:57Z

e2e/fixtures/ssg-wildcard/src/pages/[...wildcard].tsx

+export const getConfig = async () => {
+  return {
+    render: 'static',
+    staticPaths: [[], 'foo', ['bar', 'baz']],
+  };
+};


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?

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

waku/packages/waku/tests/create-pages.test.ts

Lines 60 to 67 in 10c2865

// 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',

});

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?

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?

Sure. Sounds all good!

dai-shi · 2024-05-03T23:36:10Z

packages/waku/src/router/create-pages.ts

+              break;
+            case 'group':
+              pathItems.push(staticPath[slugIndex++]!);
+              break;


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

# Conflicts: # tsconfig.e2e.json

dai-shi

Well, I don't fully read the code carefully, but let's trust the tests.

pmelab added 3 commits April 23, 2024 10:03

test: add example for wildcards in static routes

1441e48

test: e2e test case for wildcards in static routes

dd31d4d

feat: allow wildcards in static routes

0ff4a8d

vercel bot deployed to Preview April 23, 2024 08:07 View deployment

dai-shi reviewed Apr 23, 2024

View reviewed changes

pmelab added 2 commits April 23, 2024 10:28

fix: updated typing of static paths to support wildcards

3372cad

refactor: move static wildcards example to test fixtures

b633b13

pmelab requested a review from dai-shi April 23, 2024 09:06

This was referenced Apr 23, 2024

chore: set up vitest and ts-expect #667

Merged

v1 Roadmap #24

Open

pmelab added 3 commits May 1, 2024 05:30

Merge branch 'main' into static-wildcard

b085fb6

chore: upgrade react versions in ssg-wildcard fixture

2f90814

fix: include ssg-wildcard in tsconfig.e2e.json

571061f

pmelab force-pushed the static-wildcard branch from ff28b5b to 571061f Compare May 1, 2024 03:53

dai-shi reviewed May 2, 2024

View reviewed changes

pmelab added 4 commits May 2, 2024 09:33

fix: remove leftover examples dev start commands

bcc8b6b

Merge branch 'refs/heads/main' into static-wildcard

41eb3d6

# Conflicts: # pnpm-lock.yaml # tsconfig.e2e.json

test: unit test static wildcards

25604f6

fix: regression that caused static paths to receive no values

ae27243

vercel bot had a problem deploying to Preview May 3, 2024 15:14 Failure

pmelab added 2 commits May 3, 2024 17:16

chore: lockfile update

5eac498

fix: use "path" as slug name collides with the "path" route props

595c934

dai-shi reviewed May 3, 2024

View reviewed changes

Merge branch 'refs/heads/main' into static-wildcard

d56db34

# Conflicts: # tsconfig.e2e.json

vercel bot deployed to Preview May 5, 2024 09:55 View deployment

dai-shi approved these changes May 5, 2024

View reviewed changes

dai-shi merged commit 0f7204b into dai-shi:main May 5, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSG: support wildcards in static paths #666

SSG: support wildcards in static paths #666

pmelab commented Apr 23, 2024

vercel bot commented Apr 23, 2024 •

edited

Loading

codesandbox-ci bot commented Apr 23, 2024 •

edited

Loading

dai-shi commented Apr 23, 2024

dai-shi left a comment

dai-shi Apr 23, 2024

dai-shi Apr 23, 2024

pmelab Apr 23, 2024

dai-shi Apr 23, 2024

pmelab Apr 23, 2024

dai-shi Apr 23, 2024

dai-shi Apr 23, 2024

pmelab Apr 23, 2024

dai-shi May 2, 2024

pmelab May 2, 2024

pmelab May 3, 2024

dai-shi May 3, 2024

dai-shi left a comment

dai-shi May 2, 2024

himself65 May 2, 2024

dai-shi May 2, 2024

himself65 May 2, 2024

pmelab May 2, 2024

dai-shi May 2, 2024

dai-shi left a comment

dai-shi May 3, 2024

pmelab May 4, 2024

dai-shi May 4, 2024

pmelab May 5, 2024

dai-shi May 5, 2024

dai-shi May 3, 2024

dai-shi left a comment

	// 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',
	});

SSG: support wildcards in static paths #666

SSG: support wildcards in static paths #666

Conversation

pmelab commented Apr 23, 2024

vercel bot commented Apr 23, 2024 • edited Loading

codesandbox-ci bot commented Apr 23, 2024 • edited Loading

dai-shi commented Apr 23, 2024

dai-shi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

vercel bot commented Apr 23, 2024 •

edited

Loading

codesandbox-ci bot commented Apr 23, 2024 •

edited

Loading