fix: router typegen variable naming #925

rmarscher · 2024-10-01T12:50:23Z

Fixes #924

vercel · 2024-10-01T12:50:26Z

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

Name	Status	Preview	Updated (UTC)
waku	✅ Ready (Inspect)	Visit Preview	Oct 2, 2024 5:16pm

codesandbox-ci · 2024-10-01T12:50:54Z

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.

rmarscher · 2024-10-01T12:58:34Z

I'm getting 404s with this... something must be off with generated createPage calls. Updated PR to draft while I look into it.

rmarscher · 2024-10-01T13:23:12Z

This PR works fine on my cloudflare project... looking into the 404s in my aws lambda project... might be unrelated.

rmarscher · 2024-10-01T13:31:49Z

OK. My 404 on AWS was due to test code I had for the unstable_honoEnhancer config feature. I just didn't see that before because of this fs router bug. I think this PR is good to merge.

I'm having a lot of trouble testing the latest main due trouble linking the @swc/core dependency. It would be helpful to get a beta (or alpha) build of waku on npm. Thank you!

tylersayshi · 2024-10-01T19:59:50Z

@rmarscher Can you summarize what path issues in specific you were seeing as wrong and what the key changes were?

From looking at the code changed, it looks like just most of the code for the file path collection was changed. I'm not opposed to changing it at all, but it's hard to see what the precise bug and solution is for what you were seeing.

Also, filePaths was a poorly named variable. From looking back at the code, it is supposed to be /${fileNameWithExtension} in it's current form. Changing to just .endsWith is then a bit of a hack to solve for file names that are potentially incorrect to start.

Referring back to your log of filePaths in the reported issue though, those look correct I think, so i think pagesDir was actually the variable that was not correctly collected for you. I am open to some of the refactoring you did here, but I think it would be better to fix the pagesDir not getting set in response to your issue, then follow up with more changes after if we want.

apologies for this being so long 😅

rmarscher · 2024-10-01T20:01:47Z

OK. I'll create a reproduction.

rmarscher · 2024-10-01T20:32:31Z

There are two main issues that this PR addresses:

the paths are not the full relative path from the pagesDir. They are just the file name. I think you have to use path or parentPath depending on the node version - https://nodejs.org/docs/latest-v20.x/api/fs.html#direntparentpath. Since the recursive option doesn't seem very stable, I updated to a recursive function that builds the relative path as it walks the pagesDir.
multiple characters in a path that are not valid for javascript variable names are not replaced. it was only doing .replace which only replaces the first match. I updated to a global regex that will replace all.

Reproducing is pretty easy. Delete the entries.tsx and main.tsx from examples/10_fs-router and run waku dev.

For me, that causes this error:

Error: ENOENT: no such file or directory, open '/waku/examples/10_fs-router/src/pages/[name].tsx'
    at Object.openSync (node:fs:562:18)
    at readFileSync (node:fs:446:35)
    at fileExportsGetConfig (file:///waku/packages/waku/dist/lib/plugins/vite-plugin-fs-router-typegen.js:68:30)
    at generateFile (file:///waku/packages/waku/dist/lib/plugins/vite-plugin-fs-router-typegen.js:76:42)
    at updateGeneratedFile (file:///waku/packages/waku/dist/lib/plugins/vite-plugin-fs-router-typegen.js:127:51) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/waku/examples/10_fs-router/src/pages/[name].tsx'
}

I added some debug statements to updateGeneratedFile:

      const updateGeneratedFile = async () => {
        if (!pagesDir || !outputFile) return;
        const files = await collectFiles(pagesDir);
        console.log('collected files', files);
        const formatted = await formatter(generateFile(files));
        console.log('entries.gen.tsx', formatted);
        await writeFile(outputFile, formatted, 'utf-8');
      };

which outputs:

collected files [
  '/_layout.tsx',
  '/bar.tsx',
  '/index.tsx',
  '/[name].tsx',
  '/index.tsx'
]

before the error.

The error happens when calling const hasGetConfig = fileExportsGetConfig(filePath); with a filePath that doesn't exist.

I can temporarily get past the error by changing that line to const hasGetConfig = false;

Here is the entries.gen.tsx that it's trying to make:

import { createPages } from 'waku';
import type { PathsForPages } from 'waku/router';

import _Layout from './pages/_layout';
import Bar from './pages/bar';
import Index from './pages/index';
import SlugName from './pages/[name]';
import Index from './pages/index';

const _pages = createPages(async (pagesFns) => [
  pagesFns.createLayout({ path: '/', component: _Layout, render: 'static' }),
  pagesFns.createPage({ path: '/bar', component: Bar, render: 'dynamic' }),
  pagesFns.createPage({ path: '/', component: Index, render: 'dynamic' }),
  pagesFns.createPage({
    path: '/[name]',
    component: SlugName,
    render: 'dynamic',
  }),
  pagesFns.createPage({ path: '/', component: Index, render: 'dynamic' }),
]);

declare module 'waku/router' {
  interface RouteConfig {
    paths: PathsForPages<typeof _pages>;
  }
}

export default _pages;

If you rename src/pages/bar.tsx to src/pages/bar-one-two.tsx, then you will see the generated variable names can be invalid too. I get an error like this when running waku dev:

SyntaxError: '=' expected. (5:15)
  3 |
  4 | import _Layout from './pages/_layout';
> 5 | import Bar_one-two from './pages/bar-one-two';
    |               ^
  6 | import Index from './pages/index';
  7 | import SlugName from './pages/[name]';
  8 | import Index from './pages/index';
    at Q4 (file:///Users/robmarscher/Code/open-source/waku/node_modules/.pnpm/prettier@3.3.3/node_modules/prettier/plugins/typescript.mjs:17:76108)

I think it hits that error running the formatter before it saves the entries.gen.tsx.

When using the code from this PR, all is good. It looks like this:

import Layout from './pages/_layout';
import Baronetwo from './pages/bar-one-two';
import FootestIndex from './pages/foo-test/index';
import Index from './pages/index';
import NestedName from './pages/nested/[name]';

I guess technically someone could still do something to mess it up. A file named route-one.tsx and another named routeone.tsx would collide. If we replaced - with _ instead of empty string, then route-one.tsx and route_one.tsx would collide. There are probably various other things that could break it that hopefully no one would have a reason to do in practice. And if someone has that use case, then they can just make their own custom entries file.

rmarscher · 2024-10-01T20:36:01Z

Unit tests for some of these vite-plugin-fs-router-typegen.ts functions might be useful.

rmarscher · 2024-10-01T20:46:34Z

Also, filePaths was a poorly named variable. From looking back at the code, it is supposed to be /${fileNameWithExtension} in it's current form. Changing to just .endsWith is then a bit of a hack to solve for file names that are potentially incorrect to start.

Referring back to your log of filePaths in the reported issue though, those look correct I think

That doesn't make sense to me. I might be missing something. Going off the 10 fs router example, create a pages/foo/_layout.tsx file and create a pages/foo/two-levels/index.tsx file. For me, that creates an entries.gen.tsx with the following imports:

import _Layout from './pages/_layout';
import Bar from './pages/bar';
import Index from './pages/index';
import SlugName from './pages/[name]';
import _Layout from './pages/_layout';
import Index from './pages/index';
import Index from './pages/index';

Several of those paths two not resolve and there are duplicate names. After the changes in this PR, those become:

import Layout from './pages/_layout';
import Bar from './pages/bar';
import FooLayout from './pages/foo/_layout';
import FooIndex from './pages/foo/index';
import FooTwolevelsIndex from './pages/foo/two-levels/index';
import Index from './pages/index';
import NestedName from './pages/nested/[name]';

tylersayshi · 2024-10-01T22:04:21Z

import Layout from './pages/_layout';
import Bar from './pages/bar';
import FooLayout from './pages/foo/_layout';
import FooIndex from './pages/foo/index';
import FooTwolevelsIndex from './pages/foo/two-levels/index';
import Index from './pages/index';
import NestedName from './pages/nested/[name]';

This is what I initially was aiming for, so thanks for the fix.

tylersayshi · 2024-10-01T22:05:05Z

Unit tests for some of these vite-plugin-fs-router-typegen.ts functions might be useful.

agreed, would you mind adding some given that you've run into a few of the things we should test for here?

tylersayshi

Thanks for digging into this

tylersayshi · 2024-10-01T21:48:40Z

packages/waku/src/lib/plugins/vite-plugin-fs-router-typegen.ts

@@ -124,14 +110,14 @@ export const fsRouterTypegenPlugin = (opts: { srcDir: string }): Plugin => {
          const src = filePath.slice(1);
          const hasGetConfig = fileExportsGetConfig(filePath);

-          if (filePath === '/_layout.tsx') {
+          if (filePath.endsWith('/_layout.tsx')) {


Why is this needed?

same comment for both endsWith

It's needed for nested index and layout files.

ahh the filepaths are all relative to pages dir now, that makes sense

tylersayshi · 2024-10-01T21:50:22Z

packages/waku/src/lib/plugins/vite-plugin-fs-router-typegen.ts

-        for (const file of files) {
-          if (file.name.endsWith('.tsx')) {
-            results.push('/' + file.name);
+      const collectFiles = async (


@dai-shi this is going back to the manual recursive collection of files since the recursive flag seems to not be supported on node 18 if I followed correctly.

@rmarscher can you add a todo to switch this back whenever we have support for recursive: true as stable?

tylersayshi · 2024-10-01T21:52:42Z

packages/waku/src/lib/plugins/vite-plugin-fs-router-typegen.ts

@@ -6,49 +6,29 @@ import { joinPath } from '../utils/path.js';

 const SRC_PAGES = 'pages';

+const invalidCharRegex = /[^0-9a-zA-Z_$]/g;


can you add a comment for this explaining which characters are invalid for the filenames and a brief bit about why please?

To be correct... it looks like unicode chars might be allowed. And there are reserved words that need to be replaced too if they match exactly. https://tc39.es/ecma262/multipage/ecmascript-language-lexical-grammar.html#sec-names-and-keywords

sure, thanks! I meant a comment in the code with the regex though 😅 - just for anyone looking later without the PR context handy

Right. It's a good suggestion. I agree we should document it. I just wanted to chat about that first before making revisions.

I found this lib - https://github.com/eemeli/safe-identifier. It references a blog article that doesn't exist anymore that suggests appending hash of the original var name to the sanitized name to ensure that it will be unique. https://web.archive.org/web/20110703045355/https://werxltd.com/wp/2010/05/13/javascript-implementation-of-javas-string-hashcode-method/

I think I'll update it to be more correct by using a technique like that.

I also found this reference.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#identifiers
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#reserved_words

We reserved words are not an issue because we uppercase the first char.

tylersayshi · 2024-10-01T22:02:52Z

packages/waku/src/lib/plugins/vite-plugin-fs-router-typegen.ts

+        files: string[] = [],
+      ): Promise<string[]> => {
+        if (!cwd) {
+          cwd = dir;


Is cwd needed? each entry from readDir has it's parent path

dir is the starting dir and cwd is the current dir as it recurses. It needs the starting dir to create the relative path. It probably could use pagesDir from the outer scope though instead of passing around in the function.

pagesDir seems good to re-use. and I am seeing now that entry.parentPath is only available in node 20, so nvm about that.

Yeah, they started with parentPath and then changed it to just path. I tried to use the readdir recursive option when generating the _routes.json file for the Cloudflare Pages build. I found those same issues with it and pivoted to a recursive function - so it was easy for me to apply that here.

tylersayshi · 2024-10-01T22:06:20Z

packages/waku/src/lib/plugins/vite-plugin-fs-router-typegen.ts

          }
        }
-        return results;
+        return files;
      };

      const fileExportsGetConfig = (filePath: string) => {


Should filePath be renamed? or is this still right?

filePath seems right.

rmarscher · 2024-10-01T22:21:35Z

Unit tests for some of these vite-plugin-fs-router-typegen.ts functions might be useful.

agreed, would you mind adding some given that you've run into a few of the things we should test for here?

We could create a fixture for an e2e test with various pages and layout paths and names to test.

dai-shi · 2024-10-02T01:12:52Z

Unit tests for some of these vite-plugin-fs-router-typegen.ts functions might be useful.

I'm not following all the details, but it feels like unit tests are more appropriate than e2e tests. I mean unit tests for more coverage and e2e tests for some typical use cases would be good.

General idea: e2e tests are very good and necessary to tests React/RSC framework capabilities, but having more e2e tests slows down our CI, which results in bad DX. So, we prefer unit tests if possible and we are adding more (but not enough) unit tests lately.

rmarscher · 2024-10-02T01:31:17Z

Unit tests for some of these vite-plugin-fs-router-typegen.ts functions might be useful.

I'm not following all the details, but it feels like unit tests are more appropriate than e2e tests. I mean unit tests for more coverage and e2e tests for some typical use cases would be good.

General idea: e2e tests are very good and necessary to tests React/RSC framework capabilities, but having more e2e tests slows down our CI, which results in bad DX. So, we prefer unit tests if possible and we are adding more (but not enough) unit tests lately.

Great. That sounds good.

I think there are two issues - needing to use the relative fs route path when creating a list of entries and needing to convert that path into a valid javascript identifier to use as a variable name in the generated file. Tests for those two functions would be helpful. I will add vitest tests.

tylersayshi · 2024-10-02T04:24:52Z

@rmarscher wanna make sure you saw this: #925 (comment)

It would probably be good to add a check for undefined in the unit tests for the identifiers as well

Co-authored-by: Tyler <26290074+thegitduck@users.noreply.github.com>

dai-shi · 2024-10-02T12:50:32Z

packages/waku/src/lib/plugins/vite-plugin-fs-router-typegen.ts

+// from waku/router/common
+export function getInputString(path: string): string {


Can we simply import it from there? We might be changing the implementation in the future.

When I tried adding import { getInputString } from 'waku/router/common'; to the plugin, I got an error Missing "./router/common" specifier in "waku" package.

Try to use a relative path.

Try to use a relative path.

Ah right. Thanks.

rmarscher · 2024-10-02T13:29:07Z

@rmarscher wanna make sure you saw this: #925 (comment)

It would probably be good to add a check for undefined in the unit tests for the identifiers as well

I'm almost there with a new test that can catch this issue. I pushed a WIP commit but it's still failing.

…/paths-and-var-names

rmarscher · 2024-10-02T17:14:11Z

OK. This is ready again. Test coverage is much better now. I'm mocking the writeFile module to capture the write of a generated file and then checking the output contains expected text.

dai-shi

LGTM
Can @tylersayshi review?

tylersayshi · 2024-10-03T00:27:21Z

Looks great to me! thanks again :)

rmarscher · 2024-10-03T01:37:34Z

Thanks team!! 🙌 🙌

vercel bot deployed to Preview October 1, 2024 12:51 View deployment

rmarscher marked this pull request as draft October 1, 2024 12:57

rmarscher marked this pull request as ready for review October 1, 2024 13:31

fix: router typegen variable naming

00d35d4

rmarscher force-pushed the fix/fs-router-typegen/paths-and-var-names branch from 0891170 to 00d35d4 Compare October 1, 2024 15:17

vercel bot deployed to Preview October 1, 2024 15:18 View deployment

rmarscher mentioned this pull request Oct 1, 2024

waku dev error with generated fs router types #924

Closed

tylersayshi reviewed Oct 1, 2024

View reviewed changes

rmarscher marked this pull request as draft October 2, 2024 01:13

dai-shi mentioned this pull request Oct 2, 2024

v1 Roadmap #24

Open

87 tasks

Improve module name generation. Add unit tests

e7dfef7

vercel bot deployed to Preview October 2, 2024 02:35 View deployment

Use pagesDir from parent context

9bb2d88

vercel bot had a problem deploying to Preview October 2, 2024 02:37 Failure

Prettier

ba1ef41

vercel bot deployed to Preview October 2, 2024 04:24 View deployment

Cleaner getImportModuleNames

862e240

Co-authored-by: Tyler <26290074+thegitduck@users.noreply.github.com>

rmarscher force-pushed the fix/fs-router-typegen/paths-and-var-names branch from cd61d9d to 862e240 Compare October 2, 2024 04:27

vercel bot deployed to Preview October 2, 2024 04:28 View deployment

Fix key used in moduleName map

a25053e

vercel bot deployed to Preview October 2, 2024 12:32 View deployment

dai-shi reviewed Oct 2, 2024

View reviewed changes

rmarscher marked this pull request as draft October 2, 2024 13:24

WIP test vite-plugin-fs-router-typegen configureServer

16690bb

vercel bot deployed to Preview October 2, 2024 13:29 View deployment

rmarscher marked this pull request as ready for review October 2, 2024 16:59

vercel bot deployed to Preview October 2, 2024 17:01 View deployment

rmarscher marked this pull request as draft October 2, 2024 17:04

Improved vitest and importing getInputString

676ba0e

rmarscher force-pushed the fix/fs-router-typegen/paths-and-var-names branch from 1d463ba to 676ba0e Compare October 2, 2024 17:08

Merge remote-tracking branch 'origin/main' into fix/fs-router-typegen…

6461aaa

…/paths-and-var-names

vercel bot deployed to Preview October 2, 2024 17:10 View deployment

Prettier

63311ff

vercel bot deployed to Preview October 2, 2024 17:12 View deployment

rmarscher marked this pull request as ready for review October 2, 2024 17:12

Remove unnecessary fixture file

034f8dd

vercel bot deployed to Preview October 2, 2024 17:16 View deployment

dai-shi reviewed Oct 2, 2024

View reviewed changes

dai-shi merged commit f095a9f into dai-shi:main Oct 3, 2024
28 checks passed

dai-shi mentioned this pull request Nov 25, 2024

hot reloading doesn't work even in the reference example #1016

Open

		@@ -6,49 +6,29 @@ import { joinPath } from '../utils/path.js';

		const SRC_PAGES = 'pages';

		const invalidCharRegex = /[^0-9a-zA-Z_$]/g;

		// from waku/router/common
		export function getInputString(path: string): string {

fix: router typegen variable naming #925

fix: router typegen variable naming #925

Conversation

rmarscher commented Oct 1, 2024

vercel bot commented Oct 1, 2024 • edited Loading

codesandbox-ci bot commented Oct 1, 2024 • edited Loading

rmarscher commented Oct 1, 2024

rmarscher commented Oct 1, 2024

rmarscher commented Oct 1, 2024 • edited Loading

tylersayshi commented Oct 1, 2024

rmarscher commented Oct 1, 2024

rmarscher commented Oct 1, 2024 • edited Loading

rmarscher commented Oct 1, 2024

rmarscher commented Oct 1, 2024

tylersayshi commented Oct 1, 2024

tylersayshi commented Oct 1, 2024

tylersayshi 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

Choose a reason for hiding this comment

rmarscher Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarscher commented Oct 1, 2024

dai-shi commented Oct 2, 2024 • edited Loading

rmarscher commented Oct 2, 2024 • edited Loading

tylersayshi commented Oct 2, 2024

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

rmarscher commented Oct 2, 2024

rmarscher commented Oct 2, 2024

dai-shi left a comment

Choose a reason for hiding this comment

tylersayshi commented Oct 3, 2024

rmarscher commented Oct 3, 2024

vercel bot commented Oct 1, 2024 •

edited

Loading

codesandbox-ci bot commented Oct 1, 2024 •

edited

Loading

rmarscher commented Oct 1, 2024 •

edited

Loading

rmarscher commented Oct 1, 2024 •

edited

Loading

rmarscher Oct 2, 2024 •

edited

Loading

dai-shi commented Oct 2, 2024 •

edited

Loading

rmarscher commented Oct 2, 2024 •

edited

Loading