feat(react-router): don't bundle react-router in react-router/dom export
#13497
Conversation
🦋 Changeset detectedLatest commit: 818f8e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/react-router/tsup.config.ts
Outdated
| @@ -12,6 +12,7 @@ const config = (enableDevWarnings: boolean) => | |||
| { | |||
| clean: false, | |||
| entry, | |||
| external: ["react", "react-dom", "react-router"], | |||
There was a problem hiding this comment.
I only added this for the CJS config because the ESM build wasn't bundling react-router - but this can be added to both configs is that's safer
There was a problem hiding this comment.
Even if it's redundant due to the way tsup is treating the ESM build differently to the CJS build, IMO it feels safer and more explicit to define the same externals everywhere.
Also, I think react and react-dom are redundant in this array. I ran the build locally without these and the require("react") and require("react-dom") calls were still there. The tsup docs say that external is for configuring additional externals, not replacing the default: https://tsup.egoist.dev/#excluding-packages.
There was a problem hiding this comment.
ahh good catch - I noticed they weren't bundled originally but wasn't entirely sure why so included them out of an abundance of caution. Good to know that is expected/documented behavior 👍
integration/prefetch-test.ts
Outdated
| @@ -166,7 +166,8 @@ test.describe("prefetch", () => { | |||
| await page.waitForSelector( | |||
| // Look for either Rollup or Rolldown chunks | |||
| [ | |||
| "#nav link[rel='modulepreload'][href^='/assets/chunk-']", | |||
| // Rollup bundles RR (and transitively React) into an index chunk | |||
| "#nav link[rel='modulepreload'][href^='/assets/index-']", | |||
There was a problem hiding this comment.
@markdalgleish Does this feel expected or unexpected to you now that I added the external config to ESM? I'm temped to remove it so we stick with the prior ESM handling...
There was a problem hiding this comment.
As in, this change is introducing an additional chunk in the final app build? I wouldn't have guessed this would change. I've we're specifically targeting an issue with CJS build, I'm not against scoping the externals config to that build, so if you roll it back, maybe just add a comment explaining why it's not configured in the ESM build?
There was a problem hiding this comment.
I don't know that it's an additional chunk so much as a renamed chunk, but yeah I think I'll roll it back on the ESM build for now to keep the surface area minimal and we can revisit later if need be.
MichaelDeBoey
changed the title
react-router in react-router/dom export
This reverts commit fc893df.
Closes #12512
Closes #12939