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

Bundle DOM renderers into their individual UMD bundles #7164

Merged
merged 4 commits into from
Aug 9, 2016

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Jul 1, 2016

Instead of exposing the entire DOM renderer on the react.js package, I only expose CurrentOwner and ComponentTreeDevtool which are currently the only two modules that share state with the renderers.

Then I package each renderer in its own package. That could allow us to drop more server dependencies from the client package. It will also allow us to ship Fiber as a separate renderer.

Unminified DEV            after     before
react.js                  123kb     696kb
react-with-addons.js      227kb     774kb
react-dom.js              600kb     1kb
react-dom-server.js       570kb     1kb
Minified PROD             after     before
react.min.js               24kb     154kb
react-with-addons.min.js   37kb     166kb
react-dom.min.js          136kb     1kb
react-dom-server.min.js   131kb     1kb

The total size for react.min.js + react-dom.min.js is +5kb larger because of the overlap between them right now. I'd like to see what an optimizing compiler can do to this. Some of that is fbjs stuff. There shouldn't need to be that much overlap so that's something we can hunt. We should keep isomorphic absolutely minimal so there's no reason for other React clones not to use it. There will be less overlap with Fiber.

I like this strategy because it encourages us to keep isomorphic really minimal.

However, another strategy that we could do is package the isomorphic package into each renderer bundle and conditionally initialize it if it hasn't already been initialized. That way you only pay an overlap tax when there are two renderers on the page but not with one. It's also easier to just pull in one package. The downside is the versioning stuff that the separate npm package would solve. That applies to CDNs as well.

ReactWithAddons is a bit weird because it is packaged into the isomorphic package but has a bunch of DOM dependencies. So we have to load them lazily since the DOM package gets initialized after.

cc @zpao @kittens @gaearon

@sebmarkbage
Copy link
Collaborator Author

I moved ReactComponentTreeDevtool into isomorphic because it is the only dependency from isomorphic into the renderers. I'm not really sure how Perf and other devtools will work with alternative renderers since there needs to be some way to register renderers with a global devtool instead of renderer specific tools. That's a separate discussion that we've had before though.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Jul 1, 2016

Oh, this uses window.React and window.ReactDOM to access the other package. This is not fully UMD compliant, I think, so maybe I need to fix that? If we decide to go with this strategy.

@zpao
Copy link
Member

zpao commented Jul 1, 2016

Looks pretty legit. I don't think we were totally UMD compatible before either (eg you were screwed if you required the dist file of react and react-dom). Didn't look close enough at what you have here to really access but might not need to do too much.

FWIW here's the output from the compare size script:

   raw     gz Sizes
637949 143560 build/react-dom-server.js
143886  42126 build/react-dom-server.min.js
667585 150521 build/react-dom.js
148678  43694 build/react-dom.min.js
226880  50550 build/react-with-addons.js
 37020  11712 build/react-with-addons.min.js
122703  29327 build/react.js
 23694   8075 build/react.min.js

   raw     gz Compared to master @ 23cfe03c99ca3966e6f1bf6ed5f1effb06641b3c
+636750+142919build/react-dom-server.js
+143152+41681 build/react-dom-server.min.js
+666405+149889build/react-dom.js
+147963+43258 build/react-dom.min.js
-547518-122925build/react-with-addons.js
-128974-37396 build/react-with-addons.min.js
-573085-127686build/react.js
-130717-37804 build/react.min.js

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Jul 1, 2016

I managed to cut out some more shared dependencies: b04acc6

That's 3kb less on the react-dom.min.js so down to +16kb in overlap.

@sebmarkbage sebmarkbage force-pushed the separateumdbuilds branch 2 times, most recently from b04acc6 to 780e01f Compare July 2, 2016 06:52
@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Jul 2, 2016

Found a bit more unnecessary overlap. Down to 136kb for react-dom.min.js so now it is only +5kb in overlap.

null,
null,
nextElement
{ child: nextElement }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time to port devtools to new APIs 😈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still going to break the devtools with this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't break devtools but it does start showing the top level wrapper.

I switched this to an explicit flag on the wrapper type and updated devtools to support this new flag:

facebook/react-devtools#407

@zpao zpao mentioned this pull request Jul 11, 2016
4 tasks
@sebmarkbage sebmarkbage mentioned this pull request Jul 11, 2016
2 tasks
@ghost ghost added the CLA Signed label Jul 12, 2016
@yiminghe
Copy link
Contributor

yiminghe commented Jul 22, 2016

Great job, looking forward to this feature, we have put dist/react.js dist/react-dom.js into our native mobile app, so web app bundle code do not contain react core code and can load react core from local filesystem.

At the same time we build a standalone react-native bundle into our native mobile app too, and use https://github.com/react-component/rn-packager to bundle app code without react-native(react) framework code, so react-native app can also load react-native core code from local filesystem.

Currently I notice our standalone react-native bundle has some duplication with dist/react.js, we want to reuse code and reduce overall native app size.

Here is my proposal (I know it is hard for current module resolution):

react

only contains ReactElement, ReactUpdates… bundle to react.js, expose React.createElement, React._secret.ReactUpdates

react-dom

depends on react, entry:

// use ReactUpdates
const ReactUpdates = require(‘react’)._secret.ReactUpdates;
ReactUpdates.injection.xxx
// expose ReactDOM.render

react-native

depends on react, entry:

const ReactUpdates = require(‘react’)._secret.ReactUpdates;
ReactUpdates.injection.xxx
// expose ReactNative.render

@sebmarkbage
Copy link
Collaborator Author

The point of this is to put as much as possible in the individual renderer packages so that they can be versioned independently, and can be replaced by better algorithms independently. The more we share, the more we lose out on that ability.

It is an unusual scenario to have two renderers in the same app so we're optimizing for the case where you only have one renderer. However, it seems like this duplication would only marginally impact your application file size as soon as you add any significant logic to your app. Especially compared to the rest of React Native.

@yiminghe
Copy link
Contributor

understood.

As for two render, we prefer to use web inside mobile app for most situations(high dev efficiency), but in some situation(high performance) we will use react-native.

Instead of exposing the entire DOM renderer on the react.js
package, I only expose CurrentOwner and ComponentTreeDevtool which
are currently the only two modules that share __state__ with the
renderers.

Then I package each renderer in its own package. That could allow
us to drop more server dependencies from the client package. It
will also allow us to ship fiber as a separate renderer.

Unminified DEV            after     before
react.js                  123kb     696kb
react-with-addons.js      227kb     774kb
react-dom.js              668kb     1kb
react-dom-server.js       638kb     1kb

Minified PROD             after     before
react.min.js               24kb     154kb
react-with-addons.min.js   37kb     166kb
react-dom.min.js          149kb     1kb
react-dom-server.min.js   144kb     1kb

The total size for react.min.js + react-dom.min.js is +19kb larger
because of the overlap between them right now. I'd like to see
what an optimizing compiler can do to this. Some of that is fbjs
stuff. There shouldn't need to be that much overlap so that's
something we can hunt. We should keep isomorphic absolutely
minimal so there's no reason for other React clones not to use it.
There will be less overlap with Fiber.

However, another strategy that we could do is package the
isomorphic package into each renderer bundle and conditionally
initialize it if it hasn't already been initialized. That way
you only pay an overlap tax when there are two renderers on the
page but not without it. It's also easier to just pull in one
package. The downside is the versioning stuff that the separate
npm package would solve. That applies to CDNs as well.

ReactWithAddons is a bit weird because it is packaged into the
isomorphic package but has a bunch of DOM dependencies. So we have
to load them lazily since the DOM package gets initialized after.
These files reaches into isomorphic files.

The ReactElement functions are exposed on the React object anyway
so I can just use those instead.

I also found some files that are not shared that should be in
renderers shared.
renderSubtreeIntoContainer is only used by the DOM renderer.
It's not an addon.

ReactClass isn't needed as a dependency since injection doesn't
happen anymore.
By replacing this intermediate file we can do the lazy loading
without needing any lazy requires. This set up works with ES
modules.

We could also replace the globalShim thing with aliased files
instead for consistency.
var shimSharedModules = globalShim.configure({
'./ReactCurrentOwner': SECRET_INTERNALS_NAME + '.ReactCurrentOwner',
'./ReactComponentTreeHook': SECRET_INTERNALS_NAME + '.ReactComponentTreeHook',
// All these methods are shared are exposed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix that language?

@zpao
Copy link
Member

zpao commented Aug 9, 2016

Let's do it.

sebmarkbage added a commit to sebmarkbage/react-devtools that referenced this pull request Aug 9, 2016
@sebmarkbage sebmarkbage merged commit 8ef00db into facebook:master Aug 9, 2016
sophiebits pushed a commit to facebook/react-devtools that referenced this pull request Aug 9, 2016
gaearon added a commit that referenced this pull request Sep 3, 2016
Fixes #7492.
This was a build size regression introduced in #7164.
@zpao zpao modified the milestone: 15-next Sep 8, 2016
acdlite pushed a commit to acdlite/react that referenced this pull request Sep 9, 2016
@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016
zpao pushed a commit that referenced this pull request Oct 4, 2016
* Cut out isomorphic dependencies from the renderers

These files reaches into isomorphic files.

The ReactElement functions are exposed on the React object anyway
so I can just use those instead.

I also found some files that are not shared that should be in
renderers shared.

* Found a few more shared dependencies

renderSubtreeIntoContainer is only used by the DOM renderer.
It's not an addon.

ReactClass isn't needed as a dependency since injection doesn't
happen anymore.

* Use a shim file to load addons' dependencies on DOM

By replacing this intermediate file we can do the lazy loading
without needing any lazy requires. This set up works with ES
modules.

We could also replace the globalShim thing with aliased files
instead for consistency.

* Bundle DOM renderers into their individual UMD bundles

Instead of exposing the entire DOM renderer on the react.js
package, I only expose CurrentOwner and ComponentTreeDevtool which
are currently the only two modules that share __state__ with the
renderers.

Then I package each renderer in its own package. That could allow
us to drop more server dependencies from the client package. It
will also allow us to ship fiber as a separate renderer.

Unminified DEV            after     before
react.js                  123kb     696kb
react-with-addons.js      227kb     774kb
react-dom.js              668kb     1kb
react-dom-server.js       638kb     1kb

Minified PROD             after     before
react.min.js               24kb     154kb
react-with-addons.min.js   37kb     166kb
react-dom.min.js          149kb     1kb
react-dom-server.min.js   144kb     1kb

The total size for react.min.js + react-dom.min.js is +19kb larger
because of the overlap between them right now. I'd like to see
what an optimizing compiler can do to this. Some of that is fbjs
stuff. There shouldn't need to be that much overlap so that's
something we can hunt. We should keep isomorphic absolutely
minimal so there's no reason for other React clones not to use it.
There will be less overlap with Fiber.

However, another strategy that we could do is package the
isomorphic package into each renderer bundle and conditionally
initialize it if it hasn't already been initialized. That way
you only pay an overlap tax when there are two renderers on the
page but not without it. It's also easier to just pull in one
package. The downside is the versioning stuff that the separate
npm package would solve. That applies to CDNs as well.

ReactWithAddons is a bit weird because it is packaged into the
isomorphic package but has a bunch of DOM dependencies. So we have
to load them lazily since the DOM package gets initialized after.

(cherry picked from commit 8ef00db)
zpao pushed a commit that referenced this pull request Oct 4, 2016
Fixes #7492.
This was a build size regression introduced in #7164.
(cherry picked from commit a09d158)
@xgqfrms-GitHub
Copy link

old require

react commonjs require

new import

react es6 import

@sophiebits
Copy link
Collaborator

@xgqfrms-GitHub You can continue to use require() with the latest version of React. There are no changes related to that. We only changed the documentation because many newcomers are using ES6 imports.

@pixeldrew
Copy link

pixeldrew commented Nov 18, 2016

Since this PR made it to 15.4.0 I can't get require.js to load ReactDOM.render or anything from ReactDOM.

My build process is ES6 to UMD and my import statement in each of my react components looks like:

import { render } from 'react-dom';

Can confirm the fix mentioned in #8301 solves the issue

@xgqfrms-GitHub
Copy link

xgqfrms-GitHub commented Nov 19, 2016

Hooray! 🎉
We all like ES6, isn't it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants