-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore(esm/cjs): Make the router package dual & move RSC router #10957
Conversation
@Josh-Walker-GM could I get a sanity check please? Feel free to ask questions if I haven't explained my decisions clearly here! There's also a trick to get TS to not complain about async components right? I can't remember what it is now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think everything looks good. I couldn't see anything that stood out as strange or anything like that.
I don't know the TS shenanigans to get it happy with async comments. Likely a question for @Tobbe.
…-dual-pkg * 'main' of github.com:redwoodjs/redwood: (74 commits) chore(deps): Remove webpack related packages (redwoodjs#11028) fix(deps): update dependency pino to v9 (redwoodjs#11030) fix(deps): update dependency lru-cache to v11 (redwoodjs#11029) fix(deps): update dependency @sdl-codegen/node to v1 (redwoodjs#11027) chore(deps): update yarn to v4.3.1 (redwoodjs#11026) chore(deps): update dependency vitest to v2 (redwoodjs#11021) fix(deps): update dependency @react-email/render to v0.0.16 (redwoodjs#11025) chore(deps): update dependency rimraf to v6 (redwoodjs#11024) chore(deps): update dependency make-dir-cli to v4 (redwoodjs#11023) chore(deps): update dependency glob to v11 (redwoodjs#11022) chore(deps): update dependency type-fest to v4 (redwoodjs#11020) fix(deps): update dependency @whatwg-node/fetch to v0.9.18 (redwoodjs#11016) chore(deps): update dependency @supabase/ssr to v0.4.0 (redwoodjs#11017) fix(deps): update dependency @joshwooding/vite-plugin-react-docgen-typescript to v0.4.0 (redwoodjs#11018) fix(deps): update dependency esbuild to v0.23.0 (redwoodjs#11015) fix(deps): update dependency @swc/core to v1.7.0 (redwoodjs#11012) chore(storybook): Pin versions and update them (redwoodjs#11011) fix(deps): update dependency eslint-plugin-prettier to v5.2.1 (redwoodjs#11010) fix(deps): update dependency @vitejs/plugin-react to v4.3.1 (redwoodjs#11013) fix(deps): update dependency graphql-yoga to v5.6.1 (redwoodjs#11007) ...
…-dual-pkg * 'main' of github.com:redwoodjs/redwood: chore(vite): Don't exclude Router in attw check (redwoodjs#11031)
…-dual-pkg * 'main' of github.com:redwoodjs/redwood: fix(deps): update dependency ts-morph to v23 (redwoodjs#11044) chore(deps): update dependency typescript to v5.5.3 (redwoodjs#11035) Update saving-data.md (redwoodjs#11043) chore(deps): update chore (redwoodjs#11042) fix(deps): update dependency ts-morph to v22 (redwoodjs#11041) fix(deps): update dependency resend to v3 (redwoodjs#11040) chore(deps): update dependency lerna to v8.1.7 (redwoodjs#11039) fix(deps): update dependency http-proxy-middleware to v3 (redwoodjs#11038) chore(deps): update dependency ora to v8 (redwoodjs#11037) fix(deps): update dependency uuid to v10 (redwoodjs#11036) chore(deps): update dependency jsdom to v24.1.1 (redwoodjs#11033) chore(test): Document vitest issue in cli-helper test (redwoodjs#11032)
@Josh-Walker-GM headsup I think the storybook MockRouter change may break webpack-storybook, but since its not in main I can't check. I changed the milestone to next-release-major, just to be safe! |
Outstanding:
I think its probably due to types that are used across the router package that don't support async components. What I've done is "localised" the type overrides, so that they're specifically in the RSC specific code. When we are ready to update all the types to allow async components we can remove this.
Decisions I made and why
1. Put all the router stuff in the router package
I think this should be obvious why
2. If there are shared dependencies e.g. rscCss between the vite package and the router package, I put it in the router
This is to avoid a circular dependency - vite depends on router, but not the other way around. In general I think we should avoid depending on the rwjs/vite package (except for in the user's vite.config.ts)
3. importModule "sort of" duplicated
For the reason in (2) - you can't import something from vite into the router. I just put the same logic in the router package too, but specific to the modules being imported - react and rsdw/client
My assumptions are this:
a) At some point we are going to make a breaking change to rwjs/router and remove most of the things exported from the router e.g.
I've not done this now, for backwards compatibility.
b) Eventually the "RscRouter" will be imported from the root:
but for now, in RSC projects only, you need to import like this: