Use direct import to react-router.

[ruiaraujo]

Fix #5079.

[timdorr]

(Just picking one of these at random, but this applies to all the re-exports).

Unfortunately, in the packaged form, the ES module versions of these files actually live in /es/. So to get tree shaking benefits beyond these exports, you would actually have to link to react-router/es/Router. But that would break non-ES-aware bundlers.

I'm not sure if there's a good option here. If you import directly from react-router/es/Router, that is the best way to get tree-shaking benefits. Perhaps we should remove the re-exports entirely?

[ruiaraujo]

Removing re-exports would be a major change so maybe it is a bit drastic.

Also what tree shaking benefits would you hope to get that direct imports to not give you?

Since react-router mainly exports a single module from each file, tree shaking actually does not provide any value beyond removing unused modules/file which we achieve here through direct imports.

Linking to the CommonJS build by default is just the safer option but actually I think I can improve this.

[ruiaraujo]

Actually to improve on this, my initial idea was a bit of overkill.

I was thinking of a custom Babel plugin that will rewrite the export to the right direct import depending on the build.

So I decided to go the lazy route. Check it out.

[ruiaraujo]

@timdorr Any chance of taking a look at this again?

[timdorr]

Yeah, I will soon, but I have limited time available for the rest of the month. Big product release (2 years of work!) is coming up on the 30th and I need to focus on that.

[ruiaraujo]

Good luck then! 🤞

[ruiaraujo]

@timdorr Hope your release went fine. Any time available to take a look at this?

[mjackson]

Just to be clear, @ruiaraujo, this change is for people who are using babel-plugin-transform-imports with the router, right? Is that plugin not able to understand the export { Something as default } from '...' yet?

[mjackson]

Also, isn't this the kind of thing that tree-shaking was designed to help out with?

[timdorr]

Just to be clear, @ruiaraujo, this change is for people who are using babel-plugin-transform-imports with the router, right?

Nope, this is for webpack 2/3 support for ES modules. The direct re-exporting (export { Router as default } from 'react-router') is apparently not tree-shake-able by webpack. This will reduce bundle sizes significantly for those that are using the es build with webpack 2/3.

[timdorr]

BTW, this is basically our exact issue: webpack/webpack#2867 (comment) It re-exports from a file, not a npm module, but it's the same use case.

[mjackson]

Seems like a webpack bug, not ours.

[timdorr]

More like a misunderstand of what webpack means by "tree shaking". It's different from Rollup's. Hopefully the module concatenation stuff helps it out too, since it's somewhat related.

[mjackson]

So you're saying there's a blessed way to declare your imports if you want to take advantage of webpack's tree shaking? Because if you declare it like export { Something as default } from '...' webpack doesn't understand that?

[ruiaraujo]

With this import style we do not rely on tree shaking at all because the dependency is declared directly.
It does not affect consumers at all besides the fact they get smaller bundles compared to before.

[mjackson]

@ruiaraujo What do you mean by "the dependency is declared directly"? Can you please be a little more specific?

[ruiaraujo]

@mjackson If this PR is merged, react-router-dom will import MemoryRouter from react-router in practice as
import MemoryRouter from 'react-router/es/MemoryRouter' instead of
import {MemoryRouter} from 'react-router'

So this is what I mean by a directly imported dependency. Sorry for not being more explicit before.

[mjackson]

Ah, so we're using our own poor-man's tree shaking so webpack doesn't have to implement it for real? Bah. That's lame.

[ruiaraujo]

Apologies for being pragmatic in my approach. ;)

[mjackson]

Oh, I don't blame you @ruiaraujo ;) I think we can probably go ahead and merge this for now. Thanks for the PR.

[ruiaraujo]

I have rebased and fixed the conflicts.

[avocadowastaken]

Take a look to babel-plugin-direct-import, it works with RR3 and RR4.

[timdorr]

Thanks, @ruiaraujo. Based on Michael's comments, it looks like we're good to go here.