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

Convert the Source to ES Modules #11389

Merged
merged 23 commits into from Nov 2, 2017

Conversation

Projects
None yet
8 participants
@gaearon
Member

gaearon commented Oct 27, 2017

  • Fix transforms to work in both CJS and ESM modes

    • invariant
    • object-assign
    • Jest
  • Convert packages to ESM

    • react
    • react-art
    • react-call-return
    • react-test-renderer
    • react-cs-renderer
    • react-rt-renderer
    • react-noop-renderer
    • react-dom
      • server
      • client
      • events
      • shared
    • react-native-renderer
    • react-reconciler
    • events
    • shared

    Note: ESM can depend on CJS, but not the other way around. Convert things with less dependents on internals first.

  • Final cleanup

    • Remove CJS-only code in Rollup build
      • Forbid CJS
      • Make sure shims work
    • Ensure we don't regress on bundling DEV-only code
      • Perhaps, a whitelist of modules with no side effects?
      • Don't forget: lowPriorityWarning, warning, DOM hooks, DOM nesting, hyphenation utils, prop types, dev whitelists, debug things
    • Revisit the .default hack in top-level index.js files
      • Perhaps, also ESM-ify tests?
    • Consider turning some default exports into named

@gaearon gaearon requested a review from sebmarkbage Oct 31, 2017

@bvaughn bvaughn requested review from bvaughn and removed request for sebmarkbage Nov 1, 2017

@bvaughn

bvaughn approved these changes Nov 1, 2017 edited

Nice work Dan! This is a big diff! Hope I didn't overlook something important. 😁

Tests, lint, and Flow checks all pass locally. I also did a local sync to fbsource to quickly test the React Native renderer and didn't notice any obvious problems so... 👍

I wondered about the impact on bundle size of converting conditional/DEV-only requires to imports, but it looks like this PR has a mostly positive impact on bundle size. There are a couple of small size increases but they seem to be primarily in test renderer and experimental packages.

I noticed lots of new warnings during yarn build:

'default' is imported from external module '...' but never used

Looks like these are all coming from prod builds, for code that used to only be imported within a DEV conditional. It's unfortunate that this clutters up the output of build.

Show outdated Hide outdated packages/react-dom/src/client/ReactDOMFB.js
@@ -7,4 +7,5 @@
'use strict';
// TODO: this is special because it gets imported during build.

This comment has been minimized.

@bvaughn

bvaughn Nov 1, 2017

Contributor

The release script uses this, but it would be easy to change it. Looks like the only other reference is our version-check task which would also be easy to update.

I think those are the only 2 things blocking this TODO? If so, I'd be happy to help with a follow-up PR.

@bvaughn

bvaughn Nov 1, 2017

Contributor

The release script uses this, but it would be easy to change it. Looks like the only other reference is our version-check task which would also be easy to update.

I think those are the only 2 things blocking this TODO? If so, I'd be happy to help with a follow-up PR.

This comment has been minimized.

@gaearon

gaearon Nov 1, 2017

Member

Maybe the cleanest way would be to shim this file with a fake module that is generated by reading package.json.

@gaearon

gaearon Nov 1, 2017

Member

Maybe the cleanest way would be to shim this file with a fake module that is generated by reading package.json.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 1, 2017

Member

There are a couple of small size increases but they seem to be primarily in test renderer and experimental packages.

I also noticed in some cases the size before gzip decreased, but slightly increased after. Since gzip number isn’t that solid anyway (it depends on surrounding context) I wouldn’t worry too much about that.

Looks like these are all coming from prod builds, for code that used to only be imported within a DEV conditional. It's unfortunate that this clutters up the output of build.

Yeah. Basically CommonJS builds now include calls like require('warning') that don’t actually do anything. Now that I think of it, this would slightly increase the bundle size for people consuming CommonJS builds (but not increase the bundle themselves). We should find a fix for this.

Member

gaearon commented Nov 1, 2017

There are a couple of small size increases but they seem to be primarily in test renderer and experimental packages.

I also noticed in some cases the size before gzip decreased, but slightly increased after. Since gzip number isn’t that solid anyway (it depends on surrounding context) I wouldn’t worry too much about that.

Looks like these are all coming from prod builds, for code that used to only be imported within a DEV conditional. It's unfortunate that this clutters up the output of build.

Yeah. Basically CommonJS builds now include calls like require('warning') that don’t actually do anything. Now that I think of it, this would slightly increase the bundle size for people consuming CommonJS builds (but not increase the bundle themselves). We should find a fix for this.

@blling

This comment has been minimized.

Show comment
Hide comment
@blling

blling Nov 1, 2017

Is this PR ready for merge ? It is really a greate work!

blling commented Nov 1, 2017

Is this PR ready for merge ? It is really a greate work!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 1, 2017

Member

Not yet. I'm still fixing up some things.

Member

gaearon commented Nov 1, 2017

Not yet. I'm still fixing up some things.

@gaearon gaearon merged commit 21d0c11 into facebook:master Nov 2, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gaearon gaearon deleted the gaearon:esm-react branch Nov 2, 2017

@selbekk

This comment has been minimized.

Show comment
Hide comment
@selbekk

selbekk Nov 3, 2017

Contributor

Great job!

Contributor

selbekk commented Nov 3, 2017

Great job!

@Jerry-Lau

This comment has been minimized.

Show comment
Hide comment
@Jerry-Lau

Jerry-Lau commented Nov 3, 2017

Nice job!

@Jaikant

This comment has been minimized.

Show comment
Hide comment
@Jaikant

Jaikant commented Nov 3, 2017

Nice!

@tehOPEologist

This comment has been minimized.

Show comment
Hide comment
@tehOPEologist

tehOPEologist Nov 3, 2017

this didn't make it into 16.1.0-beta, did it @gaearon ? been itching to import react in browsers that support modules

tehOPEologist commented Nov 3, 2017

this didn't make it into 16.1.0-beta, did it @gaearon ? been itching to import react in browsers that support modules

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 3, 2017

Member

That's not what it is. It's not for loading React from browsers, it's changing React's own internal module system. We can do the ES build too though—just needs somebody who wants to contribute it :-)

Member

gaearon commented Nov 3, 2017

That's not what it is. It's not for loading React from browsers, it's changing React's own internal module system. We can do the ES build too though—just needs somebody who wants to contribute it :-)

@tehOPEologist

This comment has been minimized.

Show comment
Hide comment
@tehOPEologist

tehOPEologist Nov 3, 2017

ahh i see, thanks for the clarification!

tehOPEologist commented Nov 3, 2017

ahh i see, thanks for the clarification!

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Convert the Source to ES Modules (facebook#11389)
* Update transforms to handle ES modules

* Update Jest to handle ES modules

* Convert react package to ES modules

* Convert react-art package to ES Modules

* Convert react-call-return package to ES Modules

* Convert react-test-renderer package to ES Modules

* Convert react-cs-renderer package to ES Modules

* Convert react-rt-renderer package to ES Modules

* Convert react-noop-renderer package to ES Modules

* Convert react-dom/server to ES modules

* Convert react-dom/{client,events,test-utils} to ES modules

* Convert react-dom/shared to ES modules

* Convert react-native-renderer to ES modules

* Convert react-reconciler to ES modules

* Convert events to ES modules

* Convert shared to ES modules

* Remove CommonJS support from transforms

* Move ReactDOMFB entry point code into react-dom/src

This is clearer because we can use ES imports in it.

* Fix Rollup shim configuration to work with ESM

* Fix incorrect comment

* Exclude external imports without side effects

* Fix ReactDOM FB build

* Remove TODOs I don’t intend to fix yet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment