Use package.json#exports
#1185
Use package.json#exports
#1185
Conversation
🦋 Changeset detectedLatest commit: f911401 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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 |
| @@ -0,0 +1,24 @@ | |||
| --- | |||
| "@changesets/get-version-range-type": minor | |||
There was a problem hiding this comment.
Some of those are in the 0.x range so I could consider using patch for them - it doesn't quite matter for them though anyway so I'm just using minor for every pkg here
|
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 f911401:
|
Ye, based on that experience I'm totally on board with just shipping this in a minor. Can it break some people? Technically yes. Was the previous way of importing those packages in strict ESM env intentional? Not quite. Do we hear complaints from Emotion users? Not really - IMHO that's a sign that whoever we broke with this change actually welcomed the fix with arms wide open 😅 |
| "main": "dist/apply-release-plan.cjs.js", | ||
| "module": "dist/apply-release-plan.esm.js", | ||
| "main": "dist/changesets-apply-release-plan.cjs.js", | ||
| "module": "dist/changesets-apply-release-plan.esm.js", |
There was a problem hiding this comment.
this field will have no effect. it will be ignored in favor of exports and can be removed
There was a problem hiding this comment.
It's here to support older bundlers that don't support exports
There was a problem hiding this comment.
fair, I guess. Though it seems hard to imagine that people are both bundling changeset packages to create their own changeset tools and doing so with a bundler that's that old
There was a problem hiding this comment.
I agree but this is auto-generated by a somewhat universal library builder ( https://github.com/preconstruct/preconstruct/ ) and for a lot of npm packages it might still make sense to have proper compat with webpack 4 etc.
| "import": "./dist/changesets-apply-release-plan.cjs.mjs", | ||
| "default": "./dist/changesets-apply-release-plan.cjs.js" | ||
| }, | ||
| "module": "./dist/changesets-apply-release-plan.esm.js", |
There was a problem hiding this comment.
this is totally non-standard. is it really necessary? it would be much nicer not to have it
There was a problem hiding this comment.
It is totally standard :p it's just not described by node - the exports "standard" is meant to be extensible. This condition was born out of the collaboration between webpack and Rollup maintainers when it became clear that the dual package hazard is a thing. To mitigate the problem in bundlers that would like to apply node semantics closely they invented this condition that kinda works like the "old" package.json#module. Both require and import loaders in bundlers are able to load this, even though this file is authored in ESM. This is also adopted by other bundlers like Vite, bun, Parcel, esbuild and more - so I'd call it pretty standard :)
There was a problem hiding this comment.
Ah, thank you for educating me! I had no idea Rollup supported this, but it looks like it's been in @rollup/plugin-node since 11.0.0. Very much appreciate the informative responses!
| "default": "./dist/changesets-apply-release-plan.cjs.js" | ||
| }, | ||
| "module": "./dist/changesets-apply-release-plan.esm.js", | ||
| "import": "./dist/changesets-apply-release-plan.cjs.mjs", |
There was a problem hiding this comment.
I mean this in the nicest possible way, but it is absolutely crazy to use .cjs.mjs as a file extension since a file cannot be both CJS and ESM. Assuming this file is actually ESM, can the extension be just .mjs?
There was a problem hiding this comment.
This is an MJS wrapper - it's an ESM file that reexports CJS file. I know it looks funny but it kinda makes perfect sense to us. It's also kinda irrelevant rly - the extension is .mjs here. The other dotted part is not considered to be an extension, we just use it to denote some additional meaning that is separate from the package name (hence the need to use a different delimiter than a dash)
There was a problem hiding this comment.
That's helpful to know. While I agree it's irrelevant to consumers of the package, it isn't necessarily intuitive to people who might want to contribute to it. Something like -wrapper.mjs might be a little clearer
There was a problem hiding this comment.
Well, usually you just don't touch those files at all. When contributing you just interact directly with sources and all of this here is auto-generated for us. I appreciate the feedback but at the end of the day, I consider this particular thing a very minor detail that is kinda irrelevant. I can see how it might be surprising at first glance though.
| "import": "./dist/changesets-apply-release-plan.cjs.mjs", | ||
| "default": "./dist/changesets-apply-release-plan.cjs.js" | ||
| }, | ||
| "./package.json": "./package.json" |
There was a problem hiding this comment.
I'm afraid we started this convention because of some ill-behaved versions of rollup-plugin-svelte that would tell users to inform package authors to add it. It's really not necessary though and rollup-plugin-svelte no longer has that behavior
There was a problem hiding this comment.
It's to support require('pkg/package.json') in node. We think that there is no big cost associated with supporting this and some node_modules crawlers (and similar tools) are using this to inspect package metadata.
|
Super thanks, @Andarist, @emmatown and @benmccann for this "life-savør"! 😻 🥳🎉 I hit the ESM issue while being at some improvements we're working on for Changeset-Conventional-Commits and was worried at first (it was still set to |
closes #1046
closes #622