Skip to content

Conversation

@siilike
Copy link

@siilike siilike commented Nov 6, 2020

Currently in order to use the esm version of the package one needs to manually import/require the relevant thing from under esm/. This is inconvenient and doesn't work as expected in some situations.

Previously there was a pull request to add the "module" field to package.json to make tools aware where the esm modules are, but was turned down.

This pull request adds the relevant mjs files, so when the bundler/runtime is configured to prefer them over js files the esm version will be used.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To properly support ESM requires the “exports” field, and adding that is always a breaking change.

Adding the files in the interim would be semver-minor, but then they’d require using extensions, and adding the exports field would break that (since the explicit exports entry paths would ofc require extensions be omitted)

"name": "react-dates",
"version": "21.8.0",
"description": "A responsive and accessible date range picker component built with React",
"main": "index.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would break every single consumer, including ESM ones, using the specifier react-dates

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why, when it defaults to index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's a fair point, i never rely on the default so i forgot that.

That said, explicit > implicit, so altho this part isn't a breaking change, let's please revert it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but then it's not going to work, because it will always load the index.js file regardless of extension preference. Then it makes more sense to add the "exports" field instead.

Or perhaps revisit the idea of adding the "module" field? It's not standardized, but still widely used and very unlikely to break anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. The default is index.js, whether you import or require it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is index.something, node supports index.js, index.json, index.node, perhaps also something else. Webpack can be configured to prefer mjs over others, so it would use index.mjs.

But now thinking about it, it doesn't really sound like a good idea any more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not accurate - in CJS, extension resolution applies, but not in ESM. This means that index.js (in a package without type: module) is always CJS, and import('react-dates') continues to work, with or without it, by importing the CJS file.

The only resolution a package should expect is node's resolution. Using "exports", we can provide an ESM file, and then tools like webpack can use those as they like.

@ljharb ljharb added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Nov 6, 2020
@siilike siilike closed this Nov 7, 2020
@siilike siilike reopened this Nov 7, 2020
@siilike siilike closed this Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants