[Fiber] Split dev methods within ReactComponentTreeHook #9096

trueadm · 2017-03-02T18:01:29Z

ReactComponentTreeHook was originally intended to be a DEV module but has since had production usage with Fiber. This PR aims on splitting the methods out so that we don't ship extra code in production. Flow annotations were added to places that confused Flow into believing methods were missing on ReactComponentTreeHook.

Note: I'm a bit unsure about the require path remaining as react/lib/ReactComponentTreeHook though @sebmarkbage, do we still need to keep it this way?

gaearon · 2017-03-02T18:04:49Z

src/isomorphic/classic/element/ReactDebugCurrentFrame.js

@@ -14,16 +14,16 @@

 import type { Fiber } from 'ReactFiber';
 import type { DebugID } from 'ReactInstanceType';
+import type { ComponentTreeHookType } from '../../hooks/ReactComponentTreeHook';


I think this should just say from 'ReactComponentTreeHook'?

gaearon · 2017-03-02T18:06:15Z

src/isomorphic/hooks/ReactComponentTreeHook.js

-    }
-    return info;
-  },
+export type ComponentTreeHookType = {


What do you think about two separate types, one for DEV, and another for prod? Then the consumer could cast once to whichever is appropriate depending on whether we're behind DEV or not.

gaearon · 2017-03-02T18:06:38Z

src/isomorphic/classic/element/ReactDebugCurrentFrame.js

@@ -38,7 +38,9 @@ if (__DEV__) {
      if (typeof current === 'number') {
        // DebugID from Stack.
        const debugID = current;
-        stack = getStackAddendumByID(debugID);
+        if (getStackAddendumByID) {


I'd prefer to not do this since we already know we're in DEV. See my comment below on potential way to accomplish this.

This check seems unnecessary, other than for flow, since we're within a __DEV__ block. I liked Dan's suggestion below- to use a different type for dev/prod so we can avoid all of the unnecessary not-defined checks below.

gaearon · 2017-03-02T18:07:49Z

src/isomorphic/hooks/ReactComponentTreeHook.js

-    var item = getItem(id);
-    return item ? item.updateCount : 0;
-  },
+  ReactComponentTreeHook = Object.assign({}, ReactComponentTreeHook, {


Nit: why not call Object.assign() without using return value? Flow?

Flow complains if I do it any other way :)

gaearon · 2017-03-02T18:08:46Z

src/renderers/shared/ReactDebugTool.js

@@ -108,22 +118,24 @@ if (__DEV__) {
  var lifeCycleTimerHasWarned = false;

  const clearHistory = function() {
-    ReactComponentTreeHook.purgeUnmountedComponents();
+    if (purgeUnmountedComponents) {


Here, too, it seems odd to add these checks.

sophiebits

I wonder if we could have an "env: 'dev'" property on the type and then use flow refinements. I think Flow might understand this pattern too:

// in ReactComponentTreeHook
type ProdType = {env: 'prod', foo: F};
type DevType = {env: 'dev', foo: F, bar: B};
var ReactComponentTreeHook : ProdType | DevType = ...;
module.exports = ReactComponentTreeHook;

// elsewhere
const ReactComponentTreeHook = require('ReactComponentTreeHook');
if (__DEV__ && ReactComponentTreeHook.env === 'dev') {
  const ReactComponentTreeHookDev : ComponentTreeHookDevType = ReactComponentTreeHook;
  ...
}

Not entirely sure though. We should probably have this reflectable in some way though to prevent crashes from version mismatches.

sophiebits · 2017-03-02T22:08:48Z

src/isomorphic/classic/element/ReactDebugCurrentFrame.js


 const ReactDebugCurrentFrame = {};

 if (__DEV__) {
  // how do a state that ReactComponentTreeHook is using the ComponentTreeHookDevType type?
-  const ReactComponentTreeHook: ComponentTreeHookDevType = require('ReactComponentTreeHook');
+  const ReactComponentTreeHook: ComponentTreeHookDevType = (require('ReactComponentTreeHook'): any);


These look good to me. (@gaearon, I heard from @trueadm that you advised against any but it seems safe in this case.)

sophiebits · 2017-03-02T22:09:35Z

src/renderers/shared/ReactDebugTool.js

@@ -68,7 +69,7 @@ var ReactDebugTool = ((null: any): typeof ReactDebugTool);
 if (__DEV__) {
  const hooks = [];
  const didHookThrowForEvent = {};
-  const ReactComponentTreeHook = require('react/lib/ReactComponentTreeHook');
+  const ReactComponentTreeHook: ComponentTreeHookDevType = (require('react/lib/ReactComponentTreeHook'): any);


I believe this is actually not safe in the case that you use an unminified react-dom.js with react.min.js. Can you check?

Wouldn't this have failed before if that was the case?

Yes this fails: Uncaught TypeError: Cannot read property 'purgeUnmountedComponents' of undefined. What is the strategy for dealing with these issues?

It would not have failed before because we check if (purgeUnmountedComponents), etc, each time.

Oh wait I'm looking at the wrong commit.

That's what I thought, I'm a bit confused as to why they should have been removed if we don't want these errors to show when mixing bundles? I can re-add the safety checks but this will need more testing.

@gaearon @bvaughn It seems like we do want to check for each function before calling it so that react-dom.js works with react.min.js, unless you don't think that's valuable.

Oh, I didn't think about this. That's a good observation, you're right.

Good point, Ben. 👍

@spicyj @bvaughn @gaearon

I've updated the PR with some guards. Let me know if there's something I may have missed? I tested locally with React (min) and ReactDOM (non-min) and it's working, although that's not going to test all the code paths that dev code may go through – so please let me know if something doesn't look right.

sophiebits · 2017-03-02T22:14:37Z

Curious, did you see any perf changes from this?

trueadm · 2017-03-02T22:16:58Z

@spicyj It's really hard to say, the +/- % variance against a ms difference wouldn't be too accurate to report. I'll see if there is any more dev related code that can be stripped tomorrow.

sophiebits

If we're going to check function existence on every use (seems like a good idea to me), can we make the flow type reflect that by making the properties optional? Otherwise you can forget and won't get a flow error.

Files in isomorphic can actually get away without the checks since I believe they will always be in the same env. It might be nice to keep the checks there too though. In that case you might even only need one type for both dev and prod.

sebmarkbage · 2017-03-03T21:47:12Z

Not big fan of checking for existence everywhere. Seems like we can at least wrap it in something like function foo() { if (__DEV__ && doFoo) doFoo(); }. It's noisy and it should statically resolve when we compile with constant __DEV__ flags anyway so it should always be true without runtime checks.

Since we've changed strategies I expect us to break this coupling between isomorphic and renderers soon so that this can be statically resolved safely.

trueadm · 2017-03-03T22:32:28Z

@sebmarkbage I didn't check everything, I just checked a few known methods that would apply to all other DEV omitted methods. The issue with what you've said is that it still won't work if we mix prod and dev React and ReactDOM packages. That's what I already did, unless I'm missing something you're saying?

sebmarkbage · 2017-03-03T22:39:25Z

The prod version would be exporting the foo wrapper as a noop empty function.

sebmarkbage · 2017-03-03T22:43:00Z

Here's an example of a pattern we've used in a few places:

react/src/renderers/dom/shared/validateDOMNesting.js

Line 18 in 0585ee8

var validateDOMNesting = emptyFunction;

gaearon · 2017-03-03T22:43:58Z

This would bloat the production bundle with quite a few long method names.

trueadm · 2017-03-03T22:45:07Z

@sebmarkbage But this use case is a bit different right? ReactComponentTreeHook.js isn't only used in DEV anymore, the object differs because of runtime needs with dev and prod. Unless you mean we should ship an object with stubbed method names for prod? I think the best solution here is to go ahead with having checks in place, it shouldn't increase bundle size and shouldn't give us runtime errors. Maybe we can re-visit this at a later stage if we have a better overall strategy?

sebmarkbage · 2017-03-03T22:57:44Z

Normally DCE locally or with Rollup without exclude these empty functions. The only difference is if they're exposed between two packages. So we should only export the names that are actually shared between packages. That way only those names are bloating the package.

But I don't see why this is needed at all though. The only thing that needs the stack addendum in production is in Fiber. Fiber is only in the renderer. It doesn't rely on any global state.

So why doesn't the relevant code just live in the renderer or in the shared folder that gets copied to both isomorphic and renderer?

sebmarkbage · 2017-03-03T23:02:39Z

src/umd/ReactUMDEntry.js

@@ -17,17 +17,8 @@ var React = require('React');
 var ReactUMDEntry = Object.assign({
  __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: {
    ReactCurrentOwner: require('react/lib/ReactCurrentOwner'),
+    ReactComponentTreeHook: require('react/lib/ReactComponentTreeHook'),


This part is the piece I'm skeptical about.

We only needed this export because the debug ids are shared. We didn't need to export this in prod before Fiber.

Stacks through getStackAddendumByWorkInProgressFiber doesn't use the debug ids. I.e. it doesn't rely on global mutable state and therefore can be completely decoupled.

We only need prod stacks in Fiber through getStackAddendumByWorkInProgressFiber if I understand it correctly.

Therefore, why don't we just put getStackAddendumByWorkInProgressFiber in the renderer so we don't need this connection between the packages in prod?

It seems to me that if you just split up the ReactComponentTreeHook module into smaller pieces in the shared folder that both ReactComponentTreeHook and the Fiber prod stack thing can depend on the rest will fall naturally from there. ReactComponentTreeHook gets disabled in prod and so the dependencies fall off. Only the small dependencies that you split out (getStackAddendumByWorkInProgressFiber) and its dependencies get embedded in the renderer production package.

…k.js This moves the non DEV methods to a new file called ReactFiberComponentTreeHook, updates existing references to DEV functions to remain inside DEV flags so they do not pull in ReactComponentTreeHook

…ok-dev

gaearon · 2017-03-06T15:16:44Z

src/renderers/__tests__/ReactComponentTreeHook-test.js

@@ -1736,7 +1736,7 @@ describe('ReactComponentTreeHook', () => {
    expectDev(ReactComponentTreeTestUtils.getRegisteredDisplayNames()).toEqual([]);
  });

-  describe('stack addenda', () => {
+  describe.only('stack addenda', () => {


gaearon · 2017-03-07T03:35:25Z

src/isomorphic/classic/element/ReactElementValidator.js

@@ -122,7 +124,7 @@ function validateExplicitKey(element, parentType) {
    '%s%s See https://fb.me/react-warning-keys for more information.%s',
    currentComponentErrorInfo,
    childOwner,
-    ReactComponentTreeHook.getCurrentStackAddendum(element)
+    getCurrentStackAddendum && getCurrentStackAddendum(element)


Why do we need this check, here and in files below? This method is defined unconditionally, and AFAIK all the callers are also DEV-only files.

gaearon

This looks fine to me, but let's remove the && checks because they don't seem to be necessary.

sebmarkbage

Awesome! So clean.

Addressed feedback.

gaearon · 2017-03-07T13:55:22Z

🚢

trueadm added 2 commits March 2, 2017 16:15

refactor of ReactComponentTreeHook to isolate dev methods

0e23042

fixed flow issues and bug resulting in failing tests

e8f3f92

trueadm requested review from sophiebits, bvaughn, sebmarkbage, gaearon and acdlite March 2, 2017 18:01

facebook-github-bot added the CLA Signed label Mar 2, 2017

gaearon mentioned this pull request Mar 2, 2017

[Fiber] Make TreeHook DEV-only #9090

Closed

sebmarkbage mentioned this pull request Mar 2, 2017

[Fiber] Umbrella for remaining features / bugs #7925

Closed

gaearon reviewed Mar 2, 2017

View reviewed changes

trueadm added 5 commits March 2, 2017 21:50

WIP - not sure how to get the Flow type working properly

da9d918

re-added missing files

54fef50

updated flow annotations

1f7cbb6

removed comment

9a1db49

removed double semicolon

397fd2b

sophiebits requested changes Mar 2, 2017

View reviewed changes

added guards in place to prevent mixed prod+dev errors from occuring

91de17e

sophiebits previously requested changes Mar 3, 2017

View reviewed changes

sebmarkbage reviewed Mar 3, 2017

View reviewed changes

moved non DEV methods to a new file called ReactFiberComponentTreeHoo…

b572e3c

…k.js This moves the non DEV methods to a new file called ReactFiberComponentTreeHook, updates existing references to DEV functions to remain inside DEV flags so they do not pull in ReactComponentTreeHook

trueadm added 2 commits March 6, 2017 14:55

Merge remote-tracking branch 'upstream/master' into component-tree-ho…

b67a29a

…ok-dev

fixes tests so they correctly pass in fiber

af4d249

gaearon reviewed Mar 6, 2017

View reviewed changes

removed describe.only

f992d54

gaearon reviewed Mar 7, 2017

View reviewed changes

gaearon approved these changes Mar 7, 2017

View reviewed changes

sebmarkbage approved these changes Mar 7, 2017

View reviewed changes

removed inline boolean checks for methods

a159e85

trueadm merged commit 4e9d7ac into facebook:master Mar 7, 2017

gaearon deleted the component-tree-hook-dev branch March 20, 2017 16:41

[Fiber] Split dev methods within ReactComponentTreeHook #9096

[Fiber] Split dev methods within ReactComponentTreeHook #9096

Conversation

trueadm commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits commented Mar 2, 2017

trueadm commented Mar 2, 2017

sophiebits left a comment

Choose a reason for hiding this comment

sebmarkbage commented Mar 3, 2017

trueadm commented Mar 3, 2017 • edited Loading

sebmarkbage commented Mar 3, 2017

sebmarkbage commented Mar 3, 2017

gaearon commented Mar 3, 2017

trueadm commented Mar 3, 2017 • edited Loading

sebmarkbage commented Mar 3, 2017

sebmarkbage Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

gaearon commented Mar 7, 2017

trueadm Mar 2, 2017 •

edited

Loading

trueadm Mar 2, 2017 •

edited

Loading

trueadm commented Mar 3, 2017 •

edited

Loading

trueadm commented Mar 3, 2017 •

edited

Loading

sebmarkbage Mar 3, 2017 •

edited

Loading