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

Add ES Modules build #1369

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Add ES Modules build #1369

merged 1 commit into from
Feb 5, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Feb 5, 2016

Addresses #1042, #1327.

This is intended to fix the Rollup issues by fixing jsnext:main to point to the special build in the es-modules folder. That build is identical to the CommonJS build except that it doesn’t include the transform-es2015-modules-commonjs Babel plugin.

However we have a problem here. We started using Lodash 4.x recently. Lodash modules are CommonJS builds and offer no ES6 equivalents. Therefore, even with this PR, if you try to use Redux in a Rollup project with npm({ jsnext: true }), you will see the following error:

> rollup-starter-project@1.0.0 build /Users/gaearon/p/rollup-starter-project
> rollup -c rollup.config.umd.js && rollup -c rollup.config.es6.js

Module redux/node_modules/lodash/isPlainObject.js does not export default (imported by redux/es-modules/combineReducers.js)

This would be a non-issue if Lodash offered an ES modules build of its modules. For example, lodash/es-modules/isPlainObject that would be identical to lodash/isPlainObject but used ES Modules import and export instead of CommonJS.

Another possible fix is to give up on offering ES modules with Babel build, and instead build a separate ES6 single file bundle using either Webpack or Rollup itself.

I welcome comments and discussion around this because I really want to fix things for Rollup users on our end.

cc @Rich-Harris @dchambers @taion @flying-sheep

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

cc @jdalton too because wondering what Lodash stance on this is

@tyscorp
Copy link

tyscorp commented Feb 5, 2016

Lodash does export ES modules.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

😮 I had no idea, thank you!

@sebmck
Copy link

sebmck commented Feb 5, 2016

There's a lot of duplication in your .babelrc. Why can't you move all the duplicated bits to the top so the only thing in env.commonjs is the modules-commonjs plugin?

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

If we use lodash-es, how would we conditionally import either that or lodash depending on whether we are doing a CommonJS, UMD, or ES build? I’m starting to think it doesn’t make much sense to have a whole directory of es-modules. Since ES6 environments are supposed to be able to tree-shake anyway, maybe we should approach an ES build like a UMD build: a single file with ES exports and no imports. Maybe Webpack 2 (or Rollup?) could help us build that.

There's a lot of duplication in your .babelrc. Why can't you move all the duplicated bits to the top so the only thing in env.commonjs is the modules-commonjs plugin?

Haha, I assumed .babelrc is supposed to be JSON. I now realized it isn’t because I forgot quotes last time.. Will definitely extract if we solve other issues.

@glenjamin
Copy link

How much lodash do you use? Can you include the bits you do into the bundle, or do without entirely?

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

Can you include the bits you do into the bundle

Yes, definitely.

or do without entirely?

That’s how it used to be but I don’t want to maintain isPlainObject and (potentially) other similar utilities. We are using just that function currently, but we don’t plan to put it into Redux source again.

@taion
Copy link
Contributor

taion commented Feb 5, 2016

Wouldn't the right approach here be to use main: true for the npm plugin, then also use the commonjs plugin?

Realistically you're going to need both of those anyway if you want to be able to import e.g. React.

@taion
Copy link
Contributor

taion commented Feb 5, 2016

Tree-shaking for lodash here is a red herring, since you need to do the deep imports anyway to get just the parts you need for the CJS build.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

Wouldn't the right approach here be to use main: true for the npm plugin, then also use the commonjs plugin?

Dropping jsnext:main is perfectly possible but I don’t see harm in exporting an ES-first build. It doesn’t harm us, helps ES-first module bundlers like Rollup to gain more traction, and provides a build that doesn’t include the Babel interop code for environments with native modules support (future Node?).

Tree-shaking for lodash here is a red herring, since you need to do the deep imports anyway to get just the parts you need for the CJS build.

Can you expand? I’m not sure I get what you are referring to. I just want import { createStore } from 'redux' to work for Rollup users and I want them to only get createStore code in their bundle without other modules, or all the CommonJS codez both Babel or Rollup CommonJS plugin generate.

Sure, React doesn’t support jsnext:main, but it doesn’t mean we should do the same. Let’s do a step forward and learn from it. Maybe someday React will also provide an ES modules first build, or maybe jsnext:main won’t work out but some different standard will—regardless, I don’t see the future of package bundling as CommonJS.

@taion
Copy link
Contributor

taion commented Feb 5, 2016

I mean that the user needs to do e.g. https://github.com/rollup/rollup-plugin-commonjs#usage.

Something like:

  plugins: [
    npm({
      jsnext: true,
      main: true,
    }),
    commonjs({
      include: 'node_modules/**',
    }),
  ]

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

I understand. However this is a workaround. It’s necessary for libraries like React, but we don’t have to also provide only CommonJS build just because this workaround exists. The code with Rollup CommonJS plugin will be ugly because it defeats the whole purpose of Rollup, which is to use ES modules until the very end. With that solution, what you’ll end up with is a build that contains both our CommonJS interop code (generated by Babel) and Rollup’s CommonJS interop code (generated by Rollup). There is no point in doing that if we can fix it.

@taion
Copy link
Contributor

taion commented Feb 5, 2016

I think we might be talking past each other here.

My understanding of what will happen in this case is that Rollup will use jsnext:main for Redux and use the actual ES6 modules there, and not look at the CJS build of Redux at all.

It will use the CJS build for lodash, but there wouldn't be any Babel interop helpers there.

@glenjamin
Copy link

That’s how it used to be but I don’t want to maintain isPlainObject and (potentially) other similar utilities. We are using just that function currently, but we don’t plan to put it into Redux source again.

I had a look into my own question, as you say - you only use isPlainObject, and only twice.

  1. combineReducers I think I could argue that this check is overly strong. It could be watered down to "initialState and reducers must have the same top-level keys". I think this captures the spirit of the check without requiring a plain-object check
  2. dispatch in the store. I'm not entirely sure what this one is guarding against. Are you mainly worried about people passing functions, or is there a strong reason to guard against other types of object? The next check is that it must be an object with a type property - which seems like it would catch most mistakes.

I think it comes down to whether these checks are intended to catch common mistakes, or enforce some sort of rule. Personally I'd skew towards the former - and then you should be able to do without the strong isPlainObject check and rely on duck typing instead.

How good is the rollup interop, can an ES native module still depend on a small commonjs module? would using https://www.npmjs.com/package/is-plain-object help at all, or is this equivalent to using the lodash direct import?

Aside: is there a way to turn off those dispatch checks in production builds?

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

I would like to not debate depending on Lodash in this issue. It was discussed a couple of times before. I don’t want our experimental ES module bundling problems to dictate which third-party modules we use or not—this looks like a separate issue to me. (Please feel free to file an issue if you’d like to revisit any of these decisions, but they are irrelevant to providing ES modules bundle, IMO.)

@taion
Copy link
Contributor

taion commented Feb 5, 2016

If you really wanted to, you could just add a Babel transform that rewrites the lodash imports, for use in the ES2015 module build.

I don't think that's worthwhile – in practice, users are going to need to add the rollup CJS shims to work with anything else in the JS ecosystem, so you're not really saving anybody any effort.

Unless I'm missing something, they'll be getting the full benefit of ES2015 modules for Redux anyway – they just won't transitively get it for lodash as well.

@jdalton
Copy link
Contributor

jdalton commented Feb 5, 2016

Yep, lodash offers an es build. Rollup still can't natively tree shake it so you'd have to use the explicit module path to similar to what you're doing now. We have a babel plugin for converting es6 imports to node-style module paths (so you don't have to).

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

Now that I look at it, @taion is right!
This PR works well if the user has

export default {
  plugins: [babel(), npm({ jsnext: true }), commonjs({ include: 'node_modules/**',})]
};

Rollup is able to pick up Redux source code (and only import the relevant file) but it falls back to CommonJS for Lodash in this case.

@taion
Copy link
Contributor

taion commented Feb 5, 2016

You don't need an explicit main: true? Weird.

@taion
Copy link
Contributor

taion commented Feb 5, 2016

Anyway, in principle it's not ideal to require the user use the CommonJS plugin, but in practice it won't matter for some time, since that CommonJS plugin is a de facto requirement to accomplish anything useful.

This is how I have the ES2015 module builds set up for React Router and for history, and so far nobody's complained to me about it, though I also don't know if anybody's actually using said builds.

@jdalton
Copy link
Contributor

jdalton commented Feb 5, 2016

Cool! You need anything else from me?

If you use lodash-es I believe the path becomes:

import isPlainObject from 'lodash-es/isPlainObject'

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

You don't need an explicit main: true? Weird.

In this case Redux is using lodash/isPlainObject so it doesn’t have to resolve main of either Redux (it specifies jsnext:main) or Lodash (it never hits package.json due to direct import).

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

You need anything else from me?

We don’t @jdalton, we are good with using lodash-es if we decide to go with ES export. Thank you!

@taion
Copy link
Contributor

taion commented Feb 5, 2016

image

Oops.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 5, 2016

Anyway, in principle it's not ideal to require the user use the CommonJS plugin, but in practice it won't matter for some time, since that CommonJS plugin is a de facto requirement to accomplish anything useful.

Right, but IMO it’s good to set example for other libraries. Lodash does offer an ES build, and I think so should we.

@taion
Copy link
Contributor

taion commented Feb 5, 2016

I meant transitively for the lodash dependency. If you unconditionally use lodash-es everywhere, then you'd break CJS users (I think). Adding some sort of Babel plugin to rewrite Redux lodash imports to lodash-es imports doesn't seem worthwhile given that users would probably have the CommonJS Rollup plugin anyway.

"build": "npm run build:lib && npm run build:umd && npm run build:umd:min && node ./prepublish",
"prepublish": "npm run clean && npm run check:lib && npm run build",
"build:commonjs": "cross-env BABEL_ENV=commonjs babel src --out-dir lib",
"build:es": "cross-env BABEL_ENV=es babel src --out-dir es",
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question @gaearon, what (if anything) are you defining under BABEL_ENV=es? Or is it a placeholder for stuff to come or an indicator of the mode? Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just left it for consistency so I always specify either that, or commonjs. Indeed it is not currently used explicitly anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good stuff :) thanks for the clarification!
On Fri 5 Feb 2016 at 23:51 Dan Abramov notifications@github.com wrote:

In package.json
#1369 (comment):

 "check:examples": "npm run build:examples && npm run test:examples",
  • "build:lib": "babel src --out-dir lib",
  • "build:umd": "cross-env NODE_ENV=development webpack src/index.js dist/redux.js",
  • "build:umd:min": "cross-env NODE_ENV=production webpack src/index.js dist/redux.min.js",
  • "build:examples": "babel-node examples/buildAll.js",
  • "build": "npm run build:lib && npm run build:umd && npm run build:umd:min && node ./prepublish",
  • "prepublish": "npm run clean && npm run check:lib && npm run build",
  • "build:commonjs": "cross-env BABEL_ENV=commonjs babel src --out-dir lib",
  • "build:es": "cross-env BABEL_ENV=es babel src --out-dir es",

I just left it for consistency so I always specify either that, or
commonjs. Indeed it is not currently used explicitly anywhere.


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/pull/1369/files#r52088759.

@Rich-Harris
Copy link

Awesome! Thanks for making this happen @gaearon and everyone 🎉

However I’m still curious about the possibility of changing this to be a single-file build instead

There's probably no objectively correct answer. I've argued elsewhere that libraries should expose a bundled version rather than individual files, for various reasons (quicker builds for consumers, increased portability, greater resilience in the face of an evolving ecosystem), but it certainly depends on circumstance. As @jdalton and I have discovered, tree-shaking isn't a perfect science, so in some cases it makes sense to offer individual files (though I think Redux is very 'tree shakeable').

Either way, Rollup and Webpack 2 users can now create the leanest possible Redux apps with no headaches, and I'm excited about that.

FWIW I do think rewriting lodash/... imports to lodash-es/... for the es directory is a good idea, since the only safe assumption you can make about people using those files is that they can import ES modules, and even if they were able to use CommonJS modules (via Webpack 2 or Rollup with the CommonJS plugin) they'd have to pay the CommonJS overhead. It's quite possible (and increasingly so, as ES modules become more widespread) that someone would build a React/Redux app where React was the only non-ES module dependency, in which case they might serve React from a CDN:

<script src='https://cdn.jsdelivr.net/react/0.14.7/react.js'></script>
<script src='my-app-with-everything-except-react.js'></script>

@gaearon
Copy link
Contributor Author

gaearon commented Feb 6, 2016

FWIW I do think rewriting lodash/... imports to lodash-es/... for the es directory is a good idea

Easy to do so I did that in #1372, will release in 3.3.1.

@taion
Copy link
Contributor

taion commented Apr 16, 2016

What were the issues with tree-shaking with lodash-es? Trying to do some due diligence here.

Also, what approach are people using with webpack 2 support? Do you just point users to jsnext:main and tell them to configure their package resolution appropriately?

(Sorry to necro a dead thread.)

@Rich-Harris
Copy link

What were the issues with tree-shaking with lodash-es? Trying to do some due diligence here.

If you do import { whatever } from 'lodash-es', you'll import files containing stuff like this...

/** Used to detect if a method is native. */
    var reIsNative = RegExp('^' +
      funcToString.call(hasOwnProperty).replace(reRegExpChar, '\\$&')
      .replace(/hasOwnProperty|(function).*?(?=\\\()| for .+?(?=\\\])/g, '$1.*?') + '$'
    );

...which, while irrelevant to the bits that you imported, look as though (from a static analysis point of view) they might have global side-effects – unless we can somehow guarantee that the replace in .replace(...) is String.prototype.replace, which is easier said than done. So to guarantee correctness it's necessary to include that code.

Importing from individual files (import whatever from 'lodash-es/whatever') will cut down on the amount of unnecessarily included code, though it probably won't eliminate it.

@taion
Copy link
Contributor

taion commented Apr 16, 2016

Got it. That makes sense.

It sounds like this can only come up in the context of exporting a bundle as the ES module build, then – i.e. if I just provide the source modules, and my main entry point only re-exports from other modules (like https://github.com/reactjs/redux/blob/master/src/index.js and https://github.com/reactjs/react-router/blob/master/modules/index.js), then this is not a concern?

Or am I missing something there?

@Rich-Harris
Copy link

If I understand correctly, yes. It's purely down to what import (or export ... from) statements are encountered – I'm not sure about Webpack 2's behaviour, but Rollup will follow those statements and include any potentially side-effect-generating statements. So yeah, if you expose source files and there's no import chain from them to the offending files, consumers won't be affected by those side-effects as they would if they were importing from a bundle.

@taion
Copy link
Contributor

taion commented Apr 16, 2016

I'm still missing something – it looks like lodash-es does have an entry point that just re-exports: https://github.com/lodash/lodash/blob/4.11.1-es/lodash.js. I'm afraid I don't understand how that potentially causes tree-shaking issues, then.

@Rich-Harris
Copy link

Ah, sorry – missed the part about the main entry point. If you have a statement like this in the entry file...

export { default as foo } from './foo';

...then apart from it not creating a local binding in that file, it's essentially the same as this:

import foo from './foo';
export { foo };

So if a consumer wanted to import bar from that entry point, they'd be following the path to foo whether they wanted to or not, and any side-effects in that file would be included even if foo itself was unused. Your consumer would have to import directly from the file containing bar to avoid them.

@taion
Copy link
Contributor

taion commented Apr 16, 2016

Ahh! That makes sense, but it's quite unfortunate.

I don't suppose there are any proposals to add anything like passthrough re-exports?

babel-plugin-lodash is great, but I'd really like for users to be able to able to just do import { foo } from 'react-whatever'.

@Rich-Harris
Copy link

I don't suppose there are any proposals to add anything like passthrough re-exports?

Rollup actually used to work that way, but it caused problems in some cases (admittedly edge cases). I think we'd be open to revisiting it under an option, though we briefly tried (with an aggressive: true option) and it wasn't very successful – it's trickier than it sounds...

@taion
Copy link
Contributor

taion commented Apr 16, 2016

Sorry, I don't mean that this is an issue in Rollup – the way you described the semantics of the spec make sense (though they're clearly not optimal for the "re-export a bunch of stuff in entry point" use case).

I mean that I wish there were something like a

export foo from './foo' as passthrough

or whatever that would be specifically geared for this style of re-exports, which that importing bar from there would not touch any foo.

@taion
Copy link
Contributor

taion commented May 5, 2016

@Rich-Harris BTW, do you have a blog post or other explanation of this phenomenon that can be used as a reference?

@Rich-Harris
Copy link

@taion no, but I would like to!

@swernerx
Copy link

Can someone post a Webpack v2 config which deals correctly with these es6 modules? We somehow need to let a basic Babel run over these imported ES modules, right? Do you implement this with a dual Babel setup? a) local files with e.g. react plugins - b) dependent files with just es2015 modules translation.

@SimenB
Copy link
Contributor

SimenB commented Aug 22, 2016

Webpack 2 understands import, so no, you don't run babel on it. There shouldn't be any special setup necesarry for you, it's the same as with "normal" CJS build.

@swernerx
Copy link

Perfect. Thanks for letting me know.

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

Successfully merging this pull request may close these issues.