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

React Reconciler Package #10758

Merged
merged 25 commits into from
Oct 11, 2017
Merged

Conversation

iamdustan
Copy link
Contributor

@iamdustan iamdustan commented Sep 20, 2017

Alternative approach to #10641. Rather than create the entire packaging and bundling script this exposes the React Reconciler as a factory package so all private+global state is unique per reconciler which solves the original concerns raised by @gaearon in #9103 (comment).

I think it also solves the open question How should we handle React-specific stuff pretty well since it doesn’t change the current rollup/build script at this point.

I added a really cheap fixtures/reconciler/* directory that requires some manual node_modules/react-reconciler/cjs/*.js modifications to test for now, but did test both versions.

This also (rather sloppily) modifies the scripts/rollup tasks to special case the react-reconciler wrapping.

TODO:

cc @gaearon @bvaughn

@@ -0,0 +1,16 @@
# react
Copy link
Contributor Author

Choose a reason for hiding this comment

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

derp, probably need to do more than simply copy-paste this.

Copy link
Contributor

Choose a reason for hiding this comment

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

😁

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Some thoughts 😄

@@ -0,0 +1,38 @@
{
"name": "react-reconciler",
"description": "React is a JavaScript library for building user interfaces.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Prob needs better description?

* `yarn install`
* edit node_modules/react-reconciler/cjs/react-reconciler.development.js
* Add the below snipper after `var valueStack = [];`
* `yarn test`
Copy link
Contributor

@bvaughn bvaughn Sep 20, 2017

Choose a reason for hiding this comment

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

It doesn't seem ideal that our test process involves editing content in node_modules. I would expect to see something more like:

yarn install
yarn build
yarn test

Where the yarn build step just runs yarn build -- reconciler in the main project to build a copy of the reconciler.

Then the test file (index.js I guess) should test something meaningful without any editing being required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk if this needs to stick around or how to write a test that ensures that each reconciler has its own scope for reconciler-unique things like valueStack other than this really hacky way.

Possibly something with actually creating two reconcilers and then making them both do work that would indicate if they were clobbering each other or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we could automate it. At this point I don't think it's important if we manually verify it works once.

@@ -0,0 +1,16 @@
# react
Copy link
Contributor

Choose a reason for hiding this comment

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

😁

"private": true,
"dependencies": {
"react": "^16.0.0-",
"react-reconciler": "file:../../build/packages/react-reconciler"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing prop-types, fbjs, and object-assign dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷‍♂️ Everything worked okay for me so far. Idk if I even need those in the packages/react-reconciler area. There’s also a rollup warning right now for this package and apparently I broke bundling ReactDOM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops. I thought I was looking at packages/react-reconciler/package.json instead of the one in fixtures. Disregard. 😄

{
"name": "react-reconciler",
"description": "React is a JavaScript library for building user interfaces.",
"version": "16.0.0-rc.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we version this separately? Regular changes to React will be breaking changes for the reconciler.
We should either start with 0.0.1 or 1.0.0 😄

@@ -78,6 +78,7 @@ function getHeaderSanityCheck(bundleType, hasteName) {
}

function getBanner(bundleType, hasteName, filename) {
const isReconciler = /react-reconciler/.test(filename);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer a flag on the bundle, like we do for isRenderer. Or, ideally, unify them under a required enum: 'isomorphic' | 'renderer' | 'reconciler'. But unification can wait.

@gaearon
Copy link
Collaborator

gaearon commented Sep 20, 2017

Looks like this broke the core build? yarn build -- core --type=FB

@iamdustan
Copy link
Contributor Author

cc-ing @sebmarkbage / @sophiebits from convo in #9103 in case you didn’t see this PR which is more or less the model Dan was referring to at #9103 (comment)

@@ -88,10 +89,15 @@ function getBanner(bundleType, hasteName, filename) {
let banner = Header.getHeader(filename, reactVersion);
// Wrap the contents of the if-DEV check with an IIFE.
// Block-level function definitions can cause problems for strict mode.
banner += `'use strict';\n\n\nif (process.env.NODE_ENV !== "production") {\n(function() {\n`;
banner += isReconciler
? `'use strict';\n\n\nif (process.env.NODE_ENV !== "production") {\nmodule.exports = function(config) {\n`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I'm being dense but I don't understand why the reconciler needs to be a special case here. Isn't ReactFiberReconciler's export already the function we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but this covers ensuring that all other modules with global-module state is created anew for each 3rd party reconciler instance until all internal modules with that state are wrapped in factory methods.

@iamdustan
Copy link
Contributor Author

Only thing left that I know that would be nice is generating the index.js.flow file in the output package, but everything else should be covered.

@gaearon
Copy link
Collaborator

gaearon commented Sep 21, 2017

Can you fix the CI?

@iamdustan
Copy link
Contributor Author

Error: spawnSync ~/react/node_modules/.bin/prettier E2BIG
    at exports._errnoException (util.js:1022:11)
    ...

getting this error when prettier is ran. 🤔

) {
let root = container._reactRootContainer;
if (!root) {
const newRoot = Renderer.createContainer(container);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reconciler.createContainer(container);?

// Initial mount should not be batched.
Renderer.unbatchedUpdates(() => {
Renderer.updateContainer(element, newRoot, null, callback);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Renderer/Reconciler/g?

Reconciler.unbatchedUpdates(() => {
  Reconciler.updateContainer(element, newRoot, null, callback);
});

Renderer.updateContainer(element, newRoot, null, callback);
});
} else {
Renderer.updateContainer(element, root, null, callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reconciler.updateContainer(element, root, null, callback); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great eye, @koba04. thanks!

@gaearon
Copy link
Collaborator

gaearon commented Sep 22, 2017

I don't think we call the result of calling the reconciler "a reconciler" :-) Sorry for the confusion.

The result is a Renderer. Maybe the exported thing could be called RendererPublicAPI?

@bvaughn
Copy link
Contributor

bvaughn commented Sep 22, 2017

Agreed! "reconciler" may not be the most user-friendly name for the API.

Just an idea, but what about something like create-react-renderer (following the create-react-app name)? Too different from all of the other react-* names?

import createReactRenderer from 'create-react-renderer';

const Renderer = createReactRenderer(config);

@iamdustan
Copy link
Contributor Author

I would imagine a create-react-renderer to be similar to the original proposal that includes all the rollup/dev tooling to package and publish a renderer.

// external modules tell Rollup that we should not attempt
// to bundle these modules and instead treat them as
// external depedencies to the bundle. so for CJS bundles
// this means having a require("name-of-external-module") at
// the top of the bundle. for UMD bundles this means having
// both a require and a global check for them
let externalModules = externals.slice();
const isRenderer = moduleType === RENDERER || moduleType === RECONCILER;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks counter-intuitive. I would probably replace

       if (isRenderer) {
         externalModules.push('react');
       }

with

       if (moduleType !== ISOMORPHIC) {
         externalModules.push('react');
       }

below.

@@ -97,7 +101,7 @@ function getNodeModules(bundleType, isRenderer) {
return {
// Bundle object-assign once in the isomorphic React, and then use
// that from the renderer UMD. Avoids bundling it in both UMDs.
'object-assign': isRenderer
'object-assign': moduleType === RENDERER
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should instead check for ISOMORPHIC here, and reverse the ternary. Isomorphic gets the real thing, everyone else gets the shim. Even though technically we don’t have reconciler UMDs anyway.

@gaearon
Copy link
Collaborator

gaearon commented Sep 25, 2017

I'd like to figure out a way to have a testing procedure that doesn't ask folks to edit node_modules. It should be possible to write a smoke test that creates two renderers (e.g. A and B), and then renders something with A while we're inside a component being rendered by B. I think this should be enough to mess up the (shared) context stack.

@gaearon
Copy link
Collaborator

gaearon commented Sep 25, 2017

Or, perhaps, we shouldn't focus on the context at all. After all this particular case can be fixed. If you manually verified wrapping it in a closure works (why wouldn't it), this should be enough for now.

Instead we should have a fixture that lets us check that the reconciler package is not completely borked. For example by declaring a very simple renderer and writing a Node script that verifies it renders something.

@iamdustan
Copy link
Contributor Author

Do you want that done before this is merged?

Two ideas:

  • do something like what you’ve done for dev mode testing for react devtools. E.g. add an empty assignment on to the exports so we can check it from the outside.
  • have the test-renderer use this package to dogfood it and verify it works. (or just copy-paste the non-internals part of the test-renderer out into the test fixture)

@gaearon
Copy link
Collaborator

gaearon commented Sep 25, 2017

I want to have some way to test it before this is merged since we're going to start doing releases very soon. I would suggest copy-pasting noop renderer and having a simple test that verifies it works.

@iamdustan
Copy link
Contributor Author

Something like 2b196c4? It’s still not fully automatic so I could look into running the ReactIncremental-test.js there too since that is the only consumer I could find of ReactNoop in the codebase.

Also, ReactInstanceMap is private (as far as I know) so I simply commented out the findInstance method of the public interface with a comment s that the drift from the actual noop renderer has some sort of paper trail.

@gaearon
Copy link
Collaborator

gaearon commented Sep 26, 2017

I don't think we need to port the unit test. A simple smoke test verifying it doesn't instacrash rendering a simple component is enough. You can also delete all irrelevant code there.

@iamdustan
Copy link
Contributor Author

To make sure I’m understanding you, is something like this in the fixtures renderer what you’re thinking?

const Renderer = require('./noop');

expect(() => Renderer.render(<element />)).not.toThrow();

Do I need to tie that into any other command that is run during releases?

@iamdustan
Copy link
Contributor Author

@gaearon pinging in case you missed --^. Happy to make the change as soon as I know I’m doing what you expect 😄

@gaearon
Copy link
Collaborator

gaearon commented Sep 27, 2017

Noop renderer renders to a tree in memory. It would just be nice to verify basic sanity of that tree. e.g. that it created "hello world" host instance.

@dakom
Copy link

dakom commented Nov 13, 2017

woohoo, just noticed this is live: https://www.npmjs.com/package/react-reconciler :)

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.