-
Notifications
You must be signed in to change notification settings - Fork 569
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good besides the usual "technically, this could break people if they relied on .default" but it seems like that went fine for Emotion so probably ✅ ?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here to support older bundlers that don't support exports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
closes #1046
closes #622