Add feature flag to disable Fiber-only features #8757

acdlite · 2017-01-12T00:00:58Z

Disables Fiber-only render return types

sophiebits · 2017-01-12T00:40:17Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-    SynchronousPriority :
-    LowPriority;
+  let priorityContext : PriorityLevel =
+    (useSyncScheduling || ReactFeatureFlags.disableNewFiberFeatures) ?


I'm fine leaving this as before too – either way.

sophiebits · 2017-01-12T00:42:07Z

src/renderers/shared/fiber/ReactFiberReconciler.js

    updateContainer(element : ReactNodeList, container : OpaqueNode, parentComponent : ?ReactComponent<any, any, any>, callback: ?Function) : void {
+      invariant(
+        !ReactFeatureFlags.disableNewFiberFeatures || isValidElement(element),
+        'render(): Invalid component element.'


Can you make sure null/false works? And we might as well use the old error message:

invariant( inst === null || inst === false || React.isValidElement(inst), '%s(...): A valid React element (or null) must be returned. You may have ' + 'returned undefined, an array or some other invalid object.', Component.displayName || Component.name || 'Component' );

I think that's the message for when you return a non-element in a composite component. The one for top-level is here: https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactMount.js#L439

Ahh yes sorry.

acdlite · 2017-01-12T00:58:57Z

src/renderers/shared/fiber/ReactChildFiber.js

+
+      if (__DEV__) {
+        const Component = returnFiber.type;
+        warning(


Oops this should be an invariant

Well huh, it looks like sometimes Stack warns and sometimes it's an invariant?

https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js#L46

Stack both warns and does an invariant with the same message. This is an artifact from some earlier refactor when this was a different warning but has since become irrelevant. See 2646272 and feel free to reuse my other commits in #8648.

Occasionally we do that intentionally too if the warning gives a better stack.

Haha didn't see that PR, sorry Dan! I'll rebase on top

Occasionally we do that intentionally too if the warning gives a better stack.

Yep, but isn’t the case there. (Both stacks are internal and roughly in the same place.) I’m not sure what happened but I think there was some confusion during #6008 since it started as a different warning, and instead of getting removed, was changed to the same message as an invariant immediately after it.

I'll rebase on top

You probably only want da3dc03, 243d245, 2646272.

I haven't looked at your PR yet but you might want to do 8c4b2a6 and 5643a2c differently.

I'll cherry-pick everything except the last commit, in favor of the way I did it here, for the reason Sebastian gave here: #8648 (comment)

We have one for arrays but it is going to become Stack-specific.

This test is identical to "should warn when stateless component returns array" earlier. It was moved from another file in facebook#5884 so it likely survived by accident.

We have an invariant that checks the same case right afterwards. The warning was originally added in facebook#5884 with a distinct wording. However it was later changed to the same wording as the invariant in facebook#6008. I don't see why we would want to have both since they're saying the same thing and with (almost) the same internal stack.

We'll enable this in www in case we need to roll back to Stack.

Except for portals, which we use in some places.

Consider deprecating this in the future.

sptq · 2017-01-12T10:29:05Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

    });
+
+    describe('disableNewFiberFeatures', () => {
+      var ReactFeatureFlags = require('ReactFeatureFlags');


You could change var to const

We're already pretty inconsistent throughout the codebase :D One day we'll go through and clean this up

sptq · 2017-01-12T10:31:20Z

src/renderers/shared/fiber/ReactFiberReconciler.js

+var { isValidElement } = require('ReactElement');
+var invariant = require('invariant');
+var ReactFeatureFlags = require('ReactFeatureFlags');
+


You can change those to import instead of require

Actually, not all our tooling works with ES6 imports yet so we can't. require() works fine for us for now.

Moved the top-level check to ReactChildFiber like the other ones

Fixes the case where there's an uncaught error and the root unmounts. We implement this by rendering the root as if its child is null. Null is not usually allowed at the top level, so we need to special case it.

acdlite requested review from gaearon and sophiebits January 12, 2017 00:00

facebook-github-bot added CLA Signed GH Review: review-needed labels Jan 12, 2017

sophiebits requested changes Jan 12, 2017

View reviewed changes

acdlite commented Jan 12, 2017

View reviewed changes

sophiebits approved these changes Jan 12, 2017

View reviewed changes

gaearon and others added 8 commits January 11, 2017 17:10

Add a test for stateless components returning undefined

fdf0f43

We have one for arrays but it is going to become Stack-specific.

Remove a duplicate test

33c3121

This test is identical to "should warn when stateless component returns array" earlier. It was moved from another file in facebook#5884 so it likely survived by accident.

Make test that forbids arrays Stack-only

3ec1779

Add feature flag to disable Fiber-specific features

bf117f8

We'll enable this in www in case we need to roll back to Stack.

Disallow Fiber-only render return types when feature flag is on

8628018

Except for portals, which we use in some places.

Throw if undefined is returned from a composite component

204d3dd

In DEV, allow treat auto-mocked render methods as if they return null

80cf21c

Consider deprecating this in the future.

sptq suggested changes Jan 12, 2017

View reviewed changes

acdlite force-pushed the fiberfeatureflag branch from 566f8d4 to ca3497a Compare January 12, 2017 18:51

acdlite added 3 commits January 12, 2017 13:51

Top-level render only accepts a React element if feature flag is on

397810f

Warn if undefined is passed to top-level render

c8adedc

Moved the top-level check to ReactChildFiber like the other ones

Run test script and fix regressions

9459bad

Fixes the case where there's an uncaught error and the root unmounts. We implement this by rendering the root as if its child is null. Null is not usually allowed at the top level, so we need to special case it.

acdlite force-pushed the fiberfeatureflag branch from ca3497a to 9459bad Compare January 12, 2017 21:51

acdlite merged commit d28ac9e into facebook:master Jan 12, 2017

gaearon mentioned this pull request Jan 23, 2017

[Fiber] Throw on undefined component output #8648

Closed

Add feature flag to disable Fiber-only features #8757

Add feature flag to disable Fiber-only features #8757

Uh oh!

Conversation

acdlite commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaearon Jan 12, 2017 •

edited

Loading

gaearon Jan 12, 2017 •

edited

Loading