Make ES module opt-in #223
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After releasing 7.3.0, we've seen folks run into issues (#221) with the new `module` field added to package.json. I've tried to read up on how webpack uses this field, and it looks like it will pick it up by default. This is tricky because most people will expect a CommonJS export (i.e. `module.exports = ...`), but now they suddenly have to import react-waypoints using const Waypoint = require('react-waypoint').default; There's a lengthy issue over at webpack discussing this [1], with a few proposed solutions at the bottom: - offer a separate package like lodash does (lodash-es), or - skip the module field and use a es-specific entry point, like import a from 'yourpkg/es' - skip the module field, avoid es modules (my preferred solution for most of my modules) I'm opting for the second point here. We can use the commonjs bundle as the default, and then if people are adventurous they can point at the es module directly: import Waypoint from 'react-waypoint/build/index.mjs'; We're not really going to the bottom of this issue with this commit. But I believe it will solve headaches for a lot of applications. For instance, I just found out that one developer in my team spent time chasing down a bug where parts of the application wouldn't work. Waypoints that were imported with `import Waypoint from 'react-waypoint'` were working, but not the ones being imported via `const Waypoint = require('react-waypoint')` (we have some legacy). Fixes #221 [1] webpack/webpack#4742
lencioni
reviewed
Oct 5, 2017
package.json
Outdated
| @@ -3,7 +3,6 @@ | |||
| "version": "7.3.0", | |||
| "description": "A React component to execute a function whenever you scroll to an element.", | |||
| "main": "build/index.js", | |||
| "module": "build/index.mjs", | |||
There was a problem hiding this comment.
I believe the rollup config has a direct dependency on this field, so you will need to modify that as well.
There was a problem hiding this comment.
Ah, thanks. I had missed that.
Now that the target is gone from package.json, we have to specify the output file manually.
jamesplease
approved these changes
Oct 5, 2017
There was a problem hiding this comment.
The path for the ES module could possibly be nicer (for instance, import Waypoint from ‘react-waypoint/es’; but this is an improvement overall.
Thanks for the research here, and this great solution @trotzig !
I forgot that tools like Redux and other libraries that use the module field don’t have any default exports. They only have named exports, which avoids this issue.
|
I agree @jmeas, let me make that change before I merge. |
This will make importing the es module a little nicer: import Waypoint from 'react-waypoint/es'; instead of import Waypoint from 'react-waypoint/build/index.mjs'; Thanks to @jmeas for the suggestion.
|
I'm going to push this as a patch version. It could be argued that it should be major (since it's a breaking change) but I'll rather break things for people who had just adopted the es build than to force everyone on ^7.0.0 (and are now in a semi-broken state) to update to 8. |
lencioni
added a commit
to Khan/aphrodite
that referenced
this pull request
Oct 12, 2017
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223
lencioni
added a commit
to Khan/aphrodite
that referenced
this pull request
Oct 12, 2017
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223 The filesize of the dist builds before this change looked like: ``` 83K aphrodite.js 84K aphrodite.umd.js 104K aphrodite.umd.js.map 23K aphrodite.umd.min.js 204K aphrodite.umd.min.js.map ``` And after this change: ``` 72K aphrodite.js 73K aphrodite.umd.js 108K aphrodite.umd.js.map 20K aphrodite.umd.min.js 93K aphrodite.umd.min.js.map ``` So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
lencioni
added a commit
to Khan/aphrodite
that referenced
this pull request
Oct 13, 2017
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 I noticed that this Babel update caused branch coverage to drop a little, and I was unable to fix it by adding a test that definitely covered the missing branch. Thankfully, all I needed to do was add the istanbul Babel plugin to fix this. This is the approach recommended by the istanbul documentation: https://github.com/istanbuljs/nyc#use-with-babel-plugin-istanbul-for-babel-support Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223 The filesize of the dist builds before this change looked like: ``` 83K aphrodite.js 84K aphrodite.umd.js 104K aphrodite.umd.js.map 23K aphrodite.umd.min.js 204K aphrodite.umd.min.js.map ``` And after this change: ``` 72K aphrodite.js 73K aphrodite.umd.js 108K aphrodite.umd.js.map 20K aphrodite.umd.min.js 93K aphrodite.umd.min.js.map ``` So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
lencioni
added a commit
to Khan/aphrodite
that referenced
this pull request
Jan 31, 2018
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 I noticed that this Babel update caused branch coverage to drop a little, and I was unable to fix it by adding a test that definitely covered the missing branch. Thankfully, all I needed to do was add the istanbul Babel plugin to fix this. This is the approach recommended by the istanbul documentation: https://github.com/istanbuljs/nyc#use-with-babel-plugin-istanbul-for-babel-support Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223 The filesize of the dist builds before this change looked like: ``` 83K aphrodite.js 84K aphrodite.umd.js 104K aphrodite.umd.js.map 23K aphrodite.umd.min.js 204K aphrodite.umd.min.js.map ``` And after this change: ``` 72K aphrodite.js 73K aphrodite.umd.js 108K aphrodite.umd.js.map 20K aphrodite.umd.min.js 93K aphrodite.umd.min.js.map ``` So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
lencioni
added a commit
to Khan/aphrodite
that referenced
this pull request
Feb 5, 2018
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 I noticed that this Babel update caused branch coverage to drop a little, and I was unable to fix it by adding a test that definitely covered the missing branch. Thankfully, all I needed to do was add the istanbul Babel plugin to fix this. This is the approach recommended by the istanbul documentation: https://github.com/istanbuljs/nyc#use-with-babel-plugin-istanbul-for-babel-support Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223 The filesize of the dist builds before this change looked like: ``` 83K aphrodite.js 84K aphrodite.umd.js 104K aphrodite.umd.js.map 23K aphrodite.umd.min.js 204K aphrodite.umd.min.js.map ``` And after this change: ``` 72K aphrodite.js 73K aphrodite.umd.js 108K aphrodite.umd.js.map 20K aphrodite.umd.min.js 93K aphrodite.umd.min.js.map ``` So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
lencioni
added a commit
to Khan/aphrodite
that referenced
this pull request
Feb 5, 2018
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 I noticed that this Babel update caused branch coverage to drop a little, and I was unable to fix it by adding a test that definitely covered the missing branch. Thankfully, all I needed to do was add the istanbul Babel plugin to fix this. This is the approach recommended by the istanbul documentation: https://github.com/istanbuljs/nyc#use-with-babel-plugin-istanbul-for-babel-support Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223 The filesize of the dist builds before this change looked like: ``` 83K aphrodite.js 84K aphrodite.umd.js 104K aphrodite.umd.js.map 23K aphrodite.umd.min.js 204K aphrodite.umd.min.js.map ``` And after this change: ``` 72K aphrodite.js 73K aphrodite.umd.js 108K aphrodite.umd.js.map 20K aphrodite.umd.min.js 93K aphrodite.umd.min.js.map ``` So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
lencioni
added a commit
to Khan/aphrodite
that referenced
this pull request
Feb 14, 2018
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 I noticed that this Babel update caused branch coverage to drop a little, and I was unable to fix it by adding a test that definitely covered the missing branch. Thankfully, all I needed to do was add the istanbul Babel plugin to fix this. This is the approach recommended by the istanbul documentation: https://github.com/istanbuljs/nyc#use-with-babel-plugin-istanbul-for-babel-support Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223 The filesize of the dist builds before this change looked like: ``` 83K aphrodite.js 84K aphrodite.umd.js 104K aphrodite.umd.js.map 23K aphrodite.umd.min.js 204K aphrodite.umd.min.js.map ``` And after this change: ``` 72K aphrodite.js 73K aphrodite.umd.js 108K aphrodite.umd.js.map 20K aphrodite.umd.min.js 93K aphrodite.umd.min.js.map ``` So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After releasing 7.3.0, we've seen folks run into issues (#221) with the
new
modulefield added to package.json. I've tried to read up on howwebpack uses this field, and it looks like it will pick it up by
default. This is tricky because most people will expect a CommonJS
export (i.e.
module.exports = ...), but now they suddenly have toimport react-waypoints using
const Waypoint = require('react-waypoint').default;
There's a lengthy issue over at webpack discussing this [1], with a few
proposed solutions at the bottom:
I'm opting for the second point here. We can use the commonjs bundle as
the default, and then if people are adventurous they can point at the es
module directly:
import Waypoint from 'react-waypoint/build/index.mjs';
We're not really going to the bottom of this issue with this commit. But
I believe it will solve headaches for a lot of applications. For
instance, I just found out that one developer in my team spent time
chasing down a bug where parts of the application wouldn't work.
Waypoints that were imported with
import Waypoint from 'react-waypoint'were working, but not the ones being imported viaconst Waypoint = require('react-waypoint')(we have some legacy).Fixes #221
[1] webpack/webpack#4742