Generate .mjs file to allow importing from Node.js in ESM mode (#1960) #2200
Conversation
|
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. Latest deployment of this branch, based on commit a61119a:
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Would it be possible to get this merged please? |
|
Frankly, given what happened with Immer, I'm very hesitant to make any changes to our build system for the foreseeable future :( And I'm busy enough with other things that this is not at all on my priority list. At a minimum, I'd need to see someone put together some example projects that use the CodeSandbox CI build as a dependency, and show using RTK in several different environments (Node ESM, Node CJS, Next, Vite, CRA) and all working successfully, before I'd start to feel potentially comfortable merging this. |
Immer's issue was solved in a day or so, with the correct change being slated for the next major; not really much of an issue, no? Ideally avoided for sure, but wasn't a disaster. Totally understand unease, and wouldn't change this in anything but a semver major myself, but right now RTK is simply invalid w/ ESM in Node. This is a big part of the ecosystem and growing quickly, due to the actions of some maintainers who've gone ESM-only.
Keep in mind, some of these tools break from the spec rather often.... Next in particular I've seen a lot of issues in over the years. The implementation there for resolving modules seems to be far from static so would say that one should expect some amount of harmless noise from these. Probably also worth mentioning which versions of these tools you want to maintain support for? |
|
As maintainer, my number one concern is don't break existing code. I am also sympathetic with "RTK doesn't work in X environment" and want to support that. But given how fragile all this stuff is, and that I don't have time to spend on this myself, I need you or other people in the community to do the legwork here before I'm willing to merge this. I need to see working proof of this branch compiling and running okay in as many different environment setups as possible, ideally exercising all the different combinations of build outputs and module formats, so that I can see this isn't going to suddenly break things. It's also worth pointing out that while we've talked about an RTK 2.0 in #958 , and that a 2.0 would involve dropping IE11 support and updating our build formats and whatnot, we don't have concrete plans to put that out any time soon. So, either this has to be fully compatible in a minor release, or it's gotta be shelved until we are ready for a 2.0. |
|
That's totally fair, and I hope my comment didn't come off as demanding or suggesting this needs to be done! Wasn't my intention. I'm not even a Redux user fwiw (though I have enjoyed reading about it from you over the years), I don't have any need to see this land myself. I thought I'd just hop in as I've done a fair bit regarding build tools (preact-cli, wmr, microbundle, etc) and have spent way too many hours dealing with the Node exports setup in different environments. I'd definitely +1 shelving for a 2.0, even if this as-is theoretically shouldn't break any modern tools. And while I know you never get as many beta users as you'd like, this is definitely the sort of thing that'd be good to test out that way. Might just need some quick patches afterwards is what I wanted to warn about, depending on the tools and if you want to support their (often incorrect) configs. |
|
Superseded by the work I did in #3095 . |
This pull request fixes #1960 while attempting to not introduce any breaking changes.
In addition to each existing
*.esm.jsbundle, an identical*.esm.mjsbundle is created. This makes sure that existing apps/libraries that for some reason import the existing bundle by its filename don't break.An
exportsmap is added to the mainpackage.jsonof redux-toolkit. This map makes sure that Node.js will use the CommonJS bundle in CommonJS mode and the ESM bundle in ESM mode. To not break any projects that import redux-toolkit in an unconventional way, a wildcard export"./*": "./*"is added to allow importing any sub paths as before.Items for
/queryand/query/reactare also added to the exports map. So far these have been their own directories with their ownpackage.json. However, according to my research, the current layout is not supported by Node.js in ESM mode, because modules are required to live innode_modules/<package>or innode_modules/@<org>/<package>. Importing a package from a sub directory such asnode_modules/@reduxjs/toolkit/querywill raise anERR_UNSUPPORTED_DIR_IMPORTerror in Node.js. Adding these items to the exports map fixes these imports.Unless I made some mistake, this pull request should not introduce any breaking changes, so it can be published with the next patch or minor release.
For the next major release, I think the
*.esm.jsbundles can be removed. It would also be worth to consider relying exclusively on the export map and removing thequeryandquery/reactsub packages.