[Fiber] Throw on undefined component output #8648
Conversation
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.
| const nextChildren = instance.render(); | ||
| let nextChildren = instance.render(); | ||
| if (__DEV__) { | ||
| // We allow auto-mocks to proceed as if they're returning null. |
There was a problem hiding this comment.
This is copy paste from CompositeComponent.
| warnIfInvalidElement(Component, renderedElement); | ||
| if (__DEV__) { | ||
| warning( | ||
| !Component.childContextTypes, |
There was a problem hiding this comment.
This is copy paste from warnIfInvalidElement.
I changed the message to remove the wording about arrays since they're allowed in Fiber. I kept component name only in DEV since it's mostly useless with functions and classes in prod anyway when they are mangled. Alternatively we could change getComponentName() to work in DEV. I didn't want to copy and paste that snippet since, whether we include it or not, we should do it consistently in all warnings and invariants. I had to fix up a few tests that forgot to return null.
gaearon
force-pushed
the
fiber-return-undefined
branch
from
0742763 to
5643a2c
Compare
sophiebits
left a comment
sophiebits
left a comment
There was a problem hiding this comment.
I didn't actually review, but: Before we can deploy this widely at Facebook, we should add a flag that turns off the new features (returning arrays) so we can roll back if we need to.
|
Right. That should be tangential to this PR but I can keep the old error wording since we're going to put new features behind a flag. Would you prefer that? |
|
Note strictly necessary but this PR could also fix the failing diff --git a/src/renderers/dom/__tests__/ReactDOMProduction-test.js b/src/renderers/dom/__tests__/ReactDOMProduction-test.js
index ad82b35..d8fcbef 100644
--- a/src/renderers/dom/__tests__/ReactDOMProduction-test.js
+++ b/src/renderers/dom/__tests__/ReactDOMProduction-test.js
@@ -189,10 +189,17 @@ describe('ReactDOMProduction', () => {
var container = document.createElement('div');
ReactDOM.render(<Component />, container);
}).toThrowError(
- 'Minified React error #109; visit ' +
- 'http://facebook.github.io/react/docs/error-decoder.html?invariant=109&args[]=Component' +
- ' for the full message or use the non-minified dev environment' +
- ' for full errors and additional helpful warnings.'
+ ReactDOMFeatureFlags.useFiber ? (
+ 'Minified React error #150; visit ' +
+ 'http://facebook.github.io/react/docs/error-decoder.html?invariant=150&args[]=' +
+ ' for the full message or use the non-minified dev environment' +
+ ' for full errors and additional helpful warnings.'
+ ) : (
+ 'Minified React error #109; visit ' +
+ 'http://facebook.github.io/react/docs/error-decoder.html?invariant=109&args[]=Component' +
+ ' for the full message or use the non-minified dev environment' +
+ ' for full errors and additional helpful warnings.'
+ )
);
});
|
|
This is a bit woodoo.. Why are codes different? |
|
Oh, I get it, the messages are different. |
|
Yeah. (That also presumes you run |
| updateClassInstance, | ||
| } = ReactFiberClassComponent(scheduleUpdate, getPriorityContext); | ||
|
|
||
| function isValidNode(node) { |
There was a problem hiding this comment.
I don't like that this is a duplicate runtime type check with the branch check that is done in ReactChildFiber.
IMO we should move this validation into the ReactChildFiber since you can just throw if you fall out into the last branch.
There was a problem hiding this comment.
I think a good guideline is that we shouldn't have any production invariants unless the branch is already covered regardless.
|
Closing as superseded by #8757. |
5 participants
This removes an (in my opinion) unnecessary duplicate warning from Stack so that I don't have to reimplement it in Fiber. Then it changes Fiber to throw on undefined component output.
See individual commits. Only the last commit affects Fiber, the rest are tweaking Stack and tests.