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

Remove section about ES modules and tree-shaking #3410

Merged
merged 1 commit into from
Apr 29, 2016

Conversation

taion
Copy link
Contributor

@taion taion commented Apr 28, 2016

In practice, tree-shaking does not currently allow for the kind bundle size reduction achievable using direct module imports, so shouldn't encourage it as an option for minimizing bundle size.

@Rich-Harris explains it here: reduxjs/redux#1369 (comment).

Specifically, because of the way we set up the singleton histories, those will not tree-shaken out, as noted by @tomduncalf in #3387 (comment).

The best approach then is to import from the ES module directory, but I don't think we should encourage people to import from react-router/es6/* when we want to move that to react-router/es/*. Maybe we should maintain an ES module build in both /es and /es6.

In practice, tree-shaking does not currently allow for the kind of
bundle size reduction achievable using direct module imports, so we
shouldn't encourage it as an option for minimizing bundle size.
@timdorr timdorr merged commit ea7a201 into master Apr 29, 2016
@timdorr timdorr deleted the remove-tree-shaking-note branch April 29, 2016 17:38
@ryanflorence
Copy link
Member

I'm happy to rethink the singleton histories.

They showed up because of the redux/flux people wanting to navigate inside of actions.

I still think they should just pass the router to their action and do it that way, or return a promise from the action (reduxjs/redux#61 (comment)) and then in the view navigate when the action is done: this.props.dispatch(action()).then(() => router.push(...))

@taion
Copy link
Contributor Author

taion commented Apr 29, 2016

It's not just the singleton histories though, and I really do think they're a convenient pattern.

The history thing is the most noticeable example, but tree-shaking is just a fairly fragile thing right now, and doesn't work as well with top-level exports as I'd hoped.

I think it's mostly just that tree-shaking is somewhat weak in practice. I'm not even sure if it's possible to tree-shake out stuff like <Redirect> if you don't use it, since how would the bundler know that React.createClass doesn't have global side effects?

@taion
Copy link
Contributor Author

taion commented May 26, 2016

What do we think about dropping the ES module build for v3? Given that tree-shaking isn't going to work, I don't know that it's worth keeping that extra build around.

@b2whats
Copy link

b2whats commented Jun 12, 2016

yes, tree-shaking not work with react-router

main.js

// import { browserHistory } from 'react-router/es6'; //First
// import browserHistory from 'react-router/lib/browserHistory'; //Second
console.log(browserHistory)
 bundle.js      99 kB       0  [emitted]  main // First
 bundle.js    21.3 kB       0  [emitted]  main// Second

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 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

Successfully merging this pull request may close these issues.

4 participants