[transitions] Fix RTG import in ESM#48645
Conversation
Deploy previewhttps://deploy-preview-48645--material-ui.netlify.app/ Bundle size
Check out the code infra dashboard for more information about this PR. |
535ae44 to
7fec9ff
Compare
|
Here's the package diff. |
c5ca3e9 to
5e9396f
Compare
be3b553 to
4dea53d
Compare
4dea53d to
c6ba36b
Compare
|
@Janpot I think splitting import/require/default wouldn't work for the
So a more direct mapping (instead of mapping to |
|
Ok, that seems semantically correct. We also don't really need the "#mui/TransitionGroupContext": {
"node": "react-transition-group/cjs/TransitionGroupContext.js",
"default": {
"import": "react-transition-group/esm/TransitionGroupContext.js",
"require": "react-transition-group/cjs/TransitionGroupContext.js",
"default": "react-transition-group/esm/TransitionGroupContext.js",
}
} Adding tests in code-infra for this scenario. Now about codesandbox... |
|
@Janpot Without
|
|
👍 ok, makes sense |
|
Changed the fix, latest canary: @Janpot It doesn't use package |
| return `./${resolvedPath.replace('\\', '/')}`; | ||
| } | ||
|
|
||
| function rewriteTransitionGroupContextImport() { |
There was a problem hiding this comment.
this isn't needed anymore right?
There was a problem hiding this comment.
It's still needed as the first part of the fix, so that builds rewrite the import to react-transition-group/cjs/TransitionGroupContext.js for the node setups
Additionally, the "browser" field rewrites this to react-transition-group/esm/TransitionGroupContext.js for browser bundlers to handle the Webpack+CJS case
There was a problem hiding this comment.
Why not just import from react-transition-group/cjs/TransitionGroupContext.js in the source?
There was a problem hiding this comment.
Oh I didn't even think about this as a possibility
The Codex evaluation is that: it could work, but changing it in the source like that is riskier because in the repo we alias '@mui/material' to 'packages/mui-material/src', so it's a larger surface area to check for issues
Whereas this babel "plugin" is just fixing the built package, whatdo you think? @Janpot
There was a problem hiding this comment.
Whereas this babel "plugin" is just fixing the built package, whatdo you think?
tbh, that reply of codex doesn't make much sense to me. Ideally we don't mess too much with the build system. Any weird workaround only makes it harder for us to move it forward towards more modern tooling. Did you try if just the following + browser condition works?
import TransitionGroupContext from 'react-transition-group/cjs/TransitionGroupContext.js';Actually this also reminds me of a similar "fix" we did for next.js document, but that doesn't have to load in the browser.
| ) { | ||
| // Use the explicit CJS file for Node builds; package.json's `browser` | ||
| // field redirects this request to RTG's ESM file in browser bundlers. | ||
| babelPath.node.source.value = 'react-transition-group/cjs/TransitionGroupContext.js'; |
There was a problem hiding this comment.
Why can't this transform to esm for the esm build of our package instead of the current cjs in both the cases ?
The output Transition.mjs file has react-transition-group/cjs/TransitionGroupContext.js as the import path instead of esm.
There was a problem hiding this comment.
@brijeshb42 The issue is that react-transition-group/esm/TransitionGroupContext.js contains ESM syntax, but their package.json does not have "type": "module", it uses this older "module": https://github.com/reactjs/react-transition-group/blob/2989b5b87b4b4d1001f21c8efa503049ffb4fe8d/package.json#L5-L6
This causes Node 16/18 to treat it as CJS and error, does that make sense? (this is my understanding of a long Codex explanation)
There was a problem hiding this comment.
Ok. Got it. Ideally, it should have had mjs extension if no type: module in package.json
|
Best case would be to contribute the required change back to the repo. |
Fixes #48636