Add ES build to react-router and react-router-dom (#4425) #4432
Conversation
billyjanitsch
changed the title
billyjanitsch
changed the title
|
Any idea what's up with travis? The tests pass locally but the build stalls on CI. |
|
See #4430 there were errors in a fix. |
|
Thanks @kwelch -- I'll wait for that to land and rebase onto it. |
|
Landed that. Go nuts! |
| @@ -23,12 +23,14 @@ | |||
| "index.js", | |||
| "matchPath.js", | |||
| "withRouter.js", | |||
| "README.md", | |||
There was a problem hiding this comment.
Why'd you remove the README? It gets added anyways, but it's also good to make it explicit, in case we want to switch tooling to yarn or something else that differs from npm's behavior.
There was a problem hiding this comment.
Reverted. I figured it was safe to remove since there are other automatically-included files that aren't explicitly listed.
| "scripts": { | ||
| "build-lib": "babel ./modules -d . --ignore __tests__", | ||
| "build-es": "BABEL_ENV=es babel modules -d es --ignore __tests__", | ||
| "build-lib": "babel modules -d . --ignore __tests__", |
There was a problem hiding this comment.
Even though it's ignored, I'd add a BABEL_ENV=cjs to basically label this script as the CommonJS one.
|
Rebased and addressed your comments. Any thoughts on #4432 (comment)? |
|
Travis is still sad; tests still passing locally :( |
|
I dunno what was up with it. Maybe a BrowserStack issue? In any case, I'm not sure about the need for the CJS transformation. Someone can just import from the react-router package to get around that, right? |
For the simple re-exports, yes, but not for the DOM components that build on top of base ones like BrowserRouter. There won't be a way to import these in CJS without including the entire react-router package. |
| @@ -1 +1 @@ | |||
| export default from 'react-router/Prompt' | |||
| export { Prompt } from 'react-router' | |||
There was a problem hiding this comment.
Don't you mean export { Prompt as default } from 'react-router'? Otherwise, people will have to import { Prompt } from 'react-router-dom/Prompt'
| "umd" | ||
| ], | ||
| "main": "index.js", | ||
| "module": "es/index.js", |
There was a problem hiding this comment.
Consumers of the ES build (webpack 2, rollup, etc.)
|
Awesome, thanks @billyjanitsch 👍 |
This creates an ES build (still transpiled, but with ES2015 import/exports left in tact to enable tree-shaking) inside
es/, updates the relevant gitignores & npm inclusions, and adds a "module" entry point to package.json of react-router and react-router-dom.Please see #4425 (comment) before merging, as this also updates the react-router imports from react-router-dom to be top-level rather than cherry-picked (necessary to enable descendent tree-shaking). As-is, this increases the bundle size of cherry-picked CJS imports from react-router-dom, since they now include the entire react-router package.
IMO, this is fine -- if people care about bundle size, they should be using a tree-shaking bundler -- but if you disagree, I can add this babel plugin which would transpile the top-level imports back to cherry-picked ones during the CJS build.