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

Moving ES6 module from react-router/es6 to /es is a breaking change #3333

Closed
AaronFriel opened this issue Apr 16, 2016 · 8 comments
Closed

Comments

@AaronFriel
Copy link

A number of tutorials, starter kits, people's sites could be broken by this update. Here's what I could find with GitHub code search:

https://github.com/mrtnbroder/isomorphic-react-webpack-boilerplate/blob/master/src/client/index.jsx
https://github.com/fc-io/react-tape-redux/blob/master/app/containers/root.prod.js
https://github.com/nashdot/bandol/blob/master/tests/fixtures/core/module/actual.js
https://github.com/ModusCreateOrg/react-dynamic-route-loading-es6/blob/master/client/components/Toolbar/index.js
https://github.com/jaaberg/draft-js-workshop/blob/master/src/index.js

SemVer states that the major version must be updated for backwards incompatible changes to the public API. Not everyone uses rollup, jsnext:main is not a solution.

@taion
Copy link
Contributor

taion commented Apr 16, 2016

Wait, what? Is there no equivalent of jsnext:main in webpack 2?

I'd really like for the details of the ES module build (e.g. where it sits) to be an implementation detail. The documented style of ES6 imports is to import from 'react-router' directly, per https://github.com/reactjs/react-router#installation and https://github.com/reactjs/react-router/blob/master/docs/guides/MinimizingBundleSize.md#use-a-bundler-with-es2015-module-support.

Is there some way to get webpack 2 users to resolve the main for this lib appropriately? I want to have good support for people using ES modules builds, but I really don't want the import to be anything other than 'react-router' – not 'react-router/es6' or 'react-router/es'.

@ryanflorence
Copy link
Member

@AaronFriel are you personally experiencing an issue?

@taion
Copy link
Contributor

taion commented Apr 16, 2016

In the interest of not unnecessarily breaking users, we'll move the build back to es6/, subject to change on the next major release.

I'm going to update the docs to explicitly warn users away from importing from this path.

@AaronFriel
Copy link
Author

@ryanflorence: Yeah, it broke a project I'm working on and I came here trying to figure out why my imports were not working. :) It's not a huge issue, but it was a breaking change in the API.

@taion: In case you haven't yet looked this up, it's currently optional in Webpack 2 (resolve.packageMains), and won't work for a variety of people for a variety of technical and philosophical reasons:

  • It's not guaranteed what you're source and target libraries are. babel is optional, you could also use purescript, coffeescript, et al.
  • All of the above require a loader, unless the only ES6 feature you're using is imports and exports. Most configs will have their loader (e.g.: babel-loader) configured to exclude node-modules for performance. So now if it resolves jsnext:main, but babel-loader excludes your module, whoops.
  • jsnext:main is not descriptive. What happens in two years when ES2017 is the new hotness and we can depend on everyone having at least ES2015 browser support?

So the webpack community seems reluctant to do anything by default, and I don't blame them. Someone needs to propose a good solution moving forward, and there do appear to be some. It seems like the simple answer is es2015:main, typescript:main, et al.? But I digress.

@taion
Copy link
Contributor

taion commented Apr 17, 2016

Neither importing from 'react-router/es6' nor importing from 'react-router/es' are supported parts of our public API. The public API is the jsnext:main declaration.

Separately, by convention, all code published to npm (outside of React Native) space is fully transpiled to run on ES5. For almost every library, everything that ends up in lib/ is always ES5+CJS, while everything that ends up in es/ (as the newly emerging standard) is always ES5+ES6 modules.

With very few exceptions, all of node_modules is excluded from loaders. Just about every webpack setup (including the ones here) make that exclusion: https://github.com/reactjs/react-router/blob/master/webpack.config.js#L23. People that don't tend to get broken as soon as there's a Babel major version mismatch.

The issues of loaders is entirely orthogonal to the question of jsnext:main, since jsnext:main is going to be pointing at a transpiled build for the foreseeable future.

The only question is why to make the location of the build an implementation detail, because react-router/es6 is not intended to be part of the public API. You might not like jsnext:main, and honestly I don't particularly like the name either and hope a better standard will emerge, but we simply don't want users to be importing from a subdirectory.

If you want things to work reliably going forward, and to be using the officially supported option, you're going to have to add jsnext:main to mainFields.

@AaronFriel
Copy link
Author

@taion while I can hardly say for another project what is the public API, your docs did reference /es6 as an export, so that seems to be part of the public API. It looks like you've rectified that, but that's something to keep in mind.

Re: jsnext:main, your view seems to be in opposition to what the Node Foundation Core Technical Committee voted on recently. Their vote was in favor of keeping package.json the same, and adding a new extension for ES5+modules, .jsm. It's not clear to me from their vote, but I think they imagine your lib directory would include a index.jsm and it would have a higher priority than index.js.

@taion
Copy link
Contributor

taion commented Apr 17, 2016

I wrote that section of the docs. They mentioned the location of the ES module build as an implementation detail, then directed users specifically to use the 'react-router' import. At no point did we ever instruct users to use 'react-router/es6' as an import.

To avoid confusion going forward, I've updated the docs to make it more clear that users should not reach into that subdirectory for the ES module build.

Anyway, to avoid getting too far off topic, my plan here is to follow standards and conventions as much as they are available. To the extent that something official becomes available, we will use that – absent something more official, jsnext:main is as good as it gets, and as with other packages like Redux and Lodash, we will use jsnext:main until something better comes along.

@taion
Copy link
Contributor

taion commented Apr 18, 2016

Reverted on v2.3.0. Still, it's strongly recommended not to import from react-router/es6. The only supported ES module import remains to use jsnext:main and import directly from react-router.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants