-
Notifications
You must be signed in to change notification settings - Fork 973
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(router): Build with esbuild and dual esm/cjs #10685
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should provide full backwards compatibility with regards to dist imports. Added a small task to generate these instead of doing it by hand.
af8454e
to
17727c7
Compare
I think this good to go but I would need to work with @Tobbe to get the RSC specific problem solved. |
|
dac09
reviewed
Jun 4, 2024
This is just a disaster to try to debug right now. We'll come back to it soon. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
changesets-ok
Override the changesets check
release:chore
This PR is a chore (means nothing for users)
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
This PR is intended to do 2 things (because that's how every bad PR starts). First, we want to introduce the
exports
field to the package json. This will allow RSC work to start to add additional conditions to these exports. Second, we want to build and publish this package as both ESM and CJS in a non-breaking way.I have followed the previous work done in changing
@redwoodjs/auth
over to dual build ESM/CJS.To make this non-breaking we must allow all possible dist imports to continue to be imported from. This means we have to list them all in the
exports
of the package json. Whilst it is possible to use*
in these those are shallow so would have to be added for every sub-directory in dist too. I have therefore decided to be fully explicit and added every file in dist to the exports list. It may make the package json larger but being explicit is never a bad thing. We are also free to remove these at the next major and making breaking changes at that point.RSC and SSR was failing at runtime due to the dual package hazard. We would use CJS require and ESM import at different points and end up with two instances with different internal state. This was because our other packages such as
@redwoodjs/vite
which are transpiled to CJS (and so userequire
) would load the CJS variant. The project's dist and dynamic imports in our packages (which are not transpiled away) would now load the ESM variant. My stop gap solution here is to move from static imports within our framework packages (which would end up beingrequire
) to dynamic imports. In doing so we only loaded the ESM variant and avoided the dual hazard here. We can in the future convert the other packages over to dual ESM/CJS and revert the imports back to static rather than dynamic.Changes
@redwoodjs/router
to be dual built ESM/CJS.@redwoodjs/vite
to list@redwoodjs/router
in it's dependencies.@redwoodjs/router
inside the@redwoodjs/vite
package to be dynamic imports. This is not intended to be permanent.'use client'
to the splash page as it requires client side react functionality.Notes
I want to have this be
next-release
if it is possible. It would be best to start releasing and catching bugs for this change now rather than it being bundled into the next major release when it could be difficult to disambiguate the cause of bugs from other changes.I believe this is non-breaking it's current evolution but of course I could be wrong. I am slightly concerned about introducing the dual package hazard in ways that our CI has not found. In that case we can revert it and reapply it for the next major.