[Fiber] Replace throw new Error with invariant module
#8926
Conversation
| if (newChildren == null) { | ||
| throw new Error('An iterable object provided no iterator.'); | ||
| } | ||
| invariant( |
There was a problem hiding this comment.
This isn't a behavior change in your PR, but we shouldn't have any DEV-only exceptions. Can we make this a warning (either here in a new PR)?
There was a problem hiding this comment.
Good catch. There's an identical non-dev check that happens below. I'll refactor so there's only one check.
There was a problem hiding this comment.
Well, we (necessarily) call the iterator twice right? We tried to avoid that before and the code was super super messy.
There was a problem hiding this comment.
I was thinking if there's no iterator in DEV, skip the key warning and go straight to the prod invariant, which will throw.
I meant that we should make sure any internal variants clearly explain this is a bug in React and ask to file an issue. |
|
@gaearon Updated |
| @@ -72,6 +72,9 @@ const { | |||
| Deletion, | |||
| } = ReactTypeOfSideEffect; | |||
|
|
|||
| const internalErrorMessage = | |||
| 'This error is likely caused by a bug in React. Please file an issue.'; | |||
There was a problem hiding this comment.
This will count against the byte size, is this intentional?
It doesn’t matter that much in the grand scheme of things, just wondering if this was intended or not.
If you just hardcoded it everywhere then it would get stripped out.
There was a problem hiding this comment.
Hrrm, didn't think about that. Don't think it makes a substantial difference but we can change later.
There was a problem hiding this comment.
I'd prefer we do it now so it (or similar patterns) don't keep spreading.
There was a problem hiding this comment.
Me too, let's inline please.
| } | ||
| invariant( | ||
| typeof iteratorFn === 'function', | ||
| 'An object is not an iterable. (%s)', |
There was a problem hiding this comment.
Not sure we can count this as an internal error? The object is supplied by the user code.
There was a problem hiding this comment.
I think this is correct because if it's not an iterator then we shouldn't have reached this code path.
There was a problem hiding this comment.
Unless user specified code returns something different the second time we read @@iterator, no?
There was a problem hiding this comment.
That check doesn't check the type of the iterator.
| @@ -727,15 +739,17 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>( | |||
| case Fragment: | |||
| return updateFragment(current, workInProgress); | |||
| default: | |||
| throw new Error('Unknown unit of work tag'); | |||
| invariant(false, 'Unknown unit of work tag'); | |||
| throw new Error('There should always be pending or memoized props.'); | ||
| } | ||
| invariant( | ||
| newProps !== null, |
There was a problem hiding this comment.
Maybe worth checking for undefined too here and above? != null
There was a problem hiding this comment.
Yeah I'll loosen those up. Should do a pass at some point and change all our maybe types to null unions. Though props is currently an any, which is worse.
The message tells users that the error is likely due to a bug in React, and that they should file an issue.
acdlite
force-pushed
the
fiberuseinvariant
branch
from
e64de7e to
915a3e8
Compare
|
Would you inline in a follow up? |
The umbrella task says they should have "sensible messages." Does this include internal invariants (ones that only throw if there's a bug in React)?