Skip to content
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

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Use package.json#exports #1185

merged 3 commits into from
Jul 5, 2023

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Jul 3, 2023

closes #1046
closes #622

@Andarist Andarist requested a review from emmatown July 3, 2023 07:01
@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2023

🦋 Changeset detected

Latest commit: f911401

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 21 packages
Name Type
@changesets/get-version-range-type Minor
@changesets/assemble-release-plan Minor
@changesets/get-dependents-graph Minor
@changesets/apply-release-plan Minor
@changesets/changelog-github Minor
@changesets/get-release-plan Minor
@changesets/get-github-info Minor
@changesets/changelog-git Minor
@changesets/release-utils Minor
@changesets/test-utils Minor
@changesets/config Minor
@changesets/errors Minor
@changesets/logger Minor
@changesets/parse Minor
@changesets/types Minor
@changesets/write Minor
@changesets/read Minor
@changesets/cli Minor
@changesets/git Minor
@changesets/pre Minor
get-workspaces Patch

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
Copy link
Member Author

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 3, 2023

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:

Sandbox Source
Vanilla Configuration

Copy link
Member

@emmatown emmatown left a 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 ✅ ?

@Andarist
Copy link
Member Author

Andarist commented Jul 5, 2023

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 😅

@Andarist Andarist merged commit a971652 into main Jul 5, 2023
@Andarist Andarist deleted the use-exports branch July 5, 2023 06:51
@github-actions github-actions bot mentioned this pull request Jul 5, 2023
"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",
Copy link
Contributor

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

Copy link
Member Author

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

Copy link
Contributor

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

Copy link
Member Author

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",
Copy link
Contributor

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

Copy link
Member Author

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 :)

Copy link
Contributor

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",
Copy link
Contributor

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?

Copy link
Member Author

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)

Copy link
Contributor

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

Copy link
Member Author

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"
Copy link
Contributor

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

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants