Build renderers into their individual npm packages #7168

Merged
merged 1 commit into from Aug 11, 2016

Conversation

Projects
None yet
5 participants
@sebmarkbage
Member

sebmarkbage commented Jul 2, 2016

Builds on top of #7164 with one commit 7c6a95c

This copies modules into three separate packages instead of putting it all in React.

The overlap in shared folders gets duplicated.

This allows the isomorphic package to stay minimal. It can also be used as a direct dependency without much risk.

This also allow us to ship versions to each renderer independently and we can ship renderers without updating the main react package dependency.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 2, 2016

Member

The next step would be to flat bundle these.

One benefit of this is that we don't actually have to use react-native-renderer in React Native nor react-dom in FB (www). We can just copy in those source files instead of the bundle if we want.

As long as we use the react npm package as the source of truth, then third party components (minus the ones using findDOMNode), in the npm world, will just work in those environments since that's what they depend on.

I'm still a bit skeptical of the copying strategy because it delays use of smart build systems (optimizing compilers like closure compiler, emscripten for hot paths, Reason, etc) if we're limited to also getting those systems integrated into the downstreams.

Member

sebmarkbage commented Jul 2, 2016

The next step would be to flat bundle these.

One benefit of this is that we don't actually have to use react-native-renderer in React Native nor react-dom in FB (www). We can just copy in those source files instead of the bundle if we want.

As long as we use the react npm package as the source of truth, then third party components (minus the ones using findDOMNode), in the npm world, will just work in those environments since that's what they depend on.

I'm still a bit skeptical of the copying strategy because it delays use of smart build systems (optimizing compilers like closure compiler, emscripten for hot paths, Reason, etc) if we're limited to also getting those systems integrated into the downstreams.

@sebmarkbage sebmarkbage referenced this pull request Jul 2, 2016

Closed

[WIP] Gulp conversion #7172

4 of 7 tasks complete

@maxdeviant maxdeviant referenced this pull request Jul 2, 2016

Merged

Gulp: lint, flow, and version-check #7174

4 of 4 tasks complete
@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 5, 2016

Member

We might want to build the fiber renderer as its own standalone npm package that anyone can use to build a renderer. That would be great for React ART, and therefore anyone building a renderer in an external repo.

We only really need the duplication for the existing stateful DI.

Member

sebmarkbage commented Jul 5, 2016

We might want to build the fiber renderer as its own standalone npm package that anyone can use to build a renderer. That would be great for React ART, and therefore anyone building a renderer in an external repo.

We only really need the duplication for the existing stateful DI.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 5, 2016

Member

We might want to build the fiber renderer as its own standalone npm package that anyone can use to build a renderer.

👍 I think this would be amazing for learning and experimentation!

Member

gaearon commented Jul 5, 2016

We might want to build the fiber renderer as its own standalone npm package that anyone can use to build a renderer.

👍 I think this would be amazing for learning and experimentation!

sebmarkbage added a commit to sebmarkbage/react that referenced this pull request Aug 9, 2016

Include React itself in the list of shims
Without this we end up bundling all of the isomorphic React into
the DOM bundle. This was fixed in #7168 too but I'll just do an
early fix to ensure that #7168 is purely an npm change.

sebmarkbage added a commit that referenced this pull request Aug 9, 2016

Include React itself in the list of shims (#7453)
Without this we end up bundling all of the isomorphic React into
the DOM bundle. This was fixed in #7168 too but I'll just do an
early fix to ensure that #7168 is purely an npm change.
grunt/config/browserify.js
'./ReactCurrentOwner': SECRET_INTERNALS_NAME + '.ReactCurrentOwner',
'./ReactComponentTreeHook': SECRET_INTERNALS_NAME + '.ReactComponentTreeHook',
+ // All these methods are shared are exposed.

This comment has been minimized.

@zpao

zpao Aug 10, 2016

Member

rm

grunt/tasks/npm-react-addons.js
module: 'ReactComponentWithPureRenderMixin',
name: 'pure-render-mixin',
docs: 'pure-render-mixin',
},
TestUtils: {
+ package: 'react-dom',

This comment has been minimized.

@zpao

zpao Aug 10, 2016

Member

We need to account for the peerDependencies field in package.json. Right now the template has react as the dep but we'll want to override with this (or even just stop having it in the template at all and always inject - just need the version info too).

@zpao

zpao Aug 10, 2016

Member

We need to account for the peerDependencies field in package.json. Right now the template has react as the dep but we'll want to override with this (or even just stop having it in the template at all and always inject - just need the version info too).

gulpfile.js
.src(paths.react.src)
- .pipe(babel(babelOpts))
+ .pipe(babel(babelOptsReact))

This comment has been minimized.

@zpao

zpao Aug 10, 2016

Member

Nit: can you indent these lines (anything starting with the .)

@zpao

zpao Aug 10, 2016

Member

Nit: can you indent these lines (anything starting with the .)

gulpfile.js
+ return merge(
+ gulp.src(paths.react.src).pipe(extractErrors(errorCodeOpts)),
+ gulp.src(paths.reactDOM.src).pipe(extractErrors(errorCodeOpts)),
+ gulp.src(paths.reactNative.src).pipe(extractErrors(errorCodeOpts))

This comment has been minimized.

@zpao

zpao Aug 10, 2016

Member

Is this going to lead to any duplicate errors? Or issues with ordering in the future?

@zpao

zpao Aug 10, 2016

Member

Is this going to lead to any duplicate errors? Or issues with ordering in the future?

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 10, 2016

Member

I don't think this is order dependent.

I suppose this would give duplicate errors for the files in src/shared/**/*.js if they had any.

Not sure how to safely merge gulp sources since those are order dependent.

@sebmarkbage

sebmarkbage Aug 10, 2016

Member

I don't think this is order dependent.

I suppose this would give duplicate errors for the files in src/shared/**/*.js if they had any.

Not sure how to safely merge gulp sources since those are order dependent.

@@ -10,7 +10,7 @@
'use strict';
var React = require('react');
-var LinkedValueUtils = require('react/lib/LinkedValueUtils');
+var LinkedValueUtils = require('react-dom/lib/LinkedValueUtils');

This comment has been minimized.

@zpao

zpao Aug 10, 2016

Member

Probably don't need to update this package - I think this was a once and done release. (right @jimfb?)

@zpao

zpao Aug 10, 2016

Member

Probably don't need to update this package - I think this was a once and done release. (right @jimfb?)

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 10, 2016

Member

I guess that means that this is a breaking change by any standard then. If we're not planning on updating these.

Also curious about clone-with-props et al. that are out there requiring deeply.

@sebmarkbage

sebmarkbage Aug 10, 2016

Member

I guess that means that this is a breaking change by any standard then. If we're not planning on updating these.

Also curious about clone-with-props et al. that are out there requiring deeply.

This comment has been minimized.

@zpao

zpao Aug 10, 2016

Member

clone-with-props isn't a thing anymore, don't worry about it.

It's definitely a breaking change but not to a part of an API contract we've published. There will almost certainly be some packages that stop working.

I guess we could update this package in good faith, so we can leave this.

@zpao

zpao Aug 10, 2016

Member

clone-with-props isn't a thing anymore, don't worry about it.

It's definitely a breaking change but not to a part of an API contract we've published. There will almost certainly be some packages that stop working.

I guess we could update this package in good faith, so we can leave this.

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Aug 10, 2016

Member

Alright, let's do it. Let's just fix the addons packaging bit and it's good to go.

Member

zpao commented Aug 10, 2016

Alright, let's do it. Let's just fix the addons packaging bit and it's good to go.

@@ -141,7 +139,6 @@ var ReactTestRenderer = {
/* eslint-disable camelcase */
unstable_batchedUpdates: ReactUpdates.batchedUpdates,
- unstable_renderSubtreeIntoContainer: renderSubtreeIntoContainer,

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 11, 2016

Member

This shouldn't be in the test renderer according to @spicyj because this currently uses the ReactDOM dependency.

@sebmarkbage

sebmarkbage Aug 11, 2016

Member

This shouldn't be in the test renderer according to @spicyj because this currently uses the ReactDOM dependency.

Build renderers into their individual npm packages
This copies modules into three separate packages instead of
putting it all in React.

The overlap in shared and between renderers gets duplicated.

This allows the isomorphic package to stay minimal. It can also
be used as a direct dependency without much risk.

This also allow us to ship versions to each renderer independently
and we can ship renderers without updating the main react package
dependency.

@sebmarkbage sebmarkbage merged commit 0f004ef into facebook:master Aug 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iamdustan iamdustan referenced this pull request in iamdustan/tiny-react-renderer Aug 22, 2016

Open

Update for React core/renderer changes #14

@zpao zpao modified the milestone: 15-next Sep 8, 2016

@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016

zpao added a commit that referenced this pull request Oct 4, 2016

Include React itself in the list of shims (#7453)
Without this we end up bundling all of the isomorphic React into
the DOM bundle. This was fixed in #7168 too but I'll just do an
early fix to ensure that #7168 is purely an npm change.
(cherry picked from commit ca9167c)

zpao added a commit that referenced this pull request Oct 4, 2016

Build renderers into their individual npm packages (#7168)
This copies modules into three separate packages instead of
putting it all in React.

The overlap in shared and between renderers gets duplicated.

This allows the isomorphic package to stay minimal. It can also
be used as a direct dependency without much risk.

This also allow us to ship versions to each renderer independently
and we can ship renderers without updating the main react package
dependency.
(cherry picked from commit 0f004ef)
@xgqfrms-GitHub

This comment has been minimized.

Show comment
Hide comment
@xgqfrms-GitHub

xgqfrms-GitHub Nov 17, 2016

old require (CommonJS)

react commonjs require

new import (ES6)

react es6 import

old require (CommonJS)

react commonjs require

new import (ES6)

react es6 import

@renovate renovate bot referenced this pull request in signavio/backbone-rel Feb 2, 2018

Open

Update dependency react to v16 #29

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