Skip to content

[Fiber] Replace throw new Error with invariant module #8926

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 6, 2017
Merged

[Fiber] Replace throw new Error with invariant module #8926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1ef4c4
Replace `throw new Error` with invariant module
acdlite Feb 3, 2017
b338c75
Get rid of DEV only invariant
acdlite Feb 3, 2017
915a3e8
Add additional message to internal invariants
acdlite Feb 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 24 additions & 15 deletions src/renderers/shared/fiber/ReactChildFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ const {
Deletion,
} = ReactTypeOfSideEffect;

const internalErrorMessage =
'This error is likely caused by a bug in React. Please file an issue.';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hrrm, didn't think about that. Don't think it makes a substantial difference but we can change later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer we do it now so it (or similar patterns) don't keep spreading.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Me too, let's inline please.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes plz.


function coerceRef(current: ?Fiber, element: ReactElement) {
let mixedRef = element.ref;
if (mixedRef != null && typeof mixedRef !== 'function') {
Expand All @@ -88,7 +91,11 @@ function coerceRef(current: ?Fiber, element: ReactElement) {
inst = (owner : any).getPublicInstance();
}
}
invariant(inst, 'Missing owner for string ref %s', mixedRef);
invariant(
inst, 'Missing owner for string ref %s. (%s)',
mixedRef,
internalErrorMessage
);
const stringRef = String(mixedRef);
// Check if previous string ref matches new string ref
if (current && current.ref && current.ref._stringRef === stringRef) {
Expand Down Expand Up @@ -797,29 +804,31 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
// but using the iterator instead.

const iteratorFn = getIteratorFn(newChildrenIterable);
if (typeof iteratorFn !== 'function') {
throw new Error('An object is not an iterable.');
}
invariant(
typeof iteratorFn === 'function',
'An object is not an iterable. (%s)',
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 we can count this as an internal error? The object is supplied by the user code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, my bad

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is correct because if it's not an iterator then we shouldn't have reached this code path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless user specified code returns something different the second time we read @@iterator, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be a getter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That check doesn't check the type of the iterator.

internalErrorMessage
);

if (__DEV__) {
// First, validate keys.
// We'll get a different iterator later for the main pass.
const newChildren = iteratorFn.call(newChildrenIterable);
if (newChildren == null) {
throw new Error('An iterable object provided no iterator.');
}
let knownKeys = null;
let step = newChildren.next();
for (; !step.done; step = newChildren.next()) {
const child = step.value;
knownKeys = warnOnDuplicateKey(child, knownKeys);
if (newChildren) {
let knownKeys = null;
let step = newChildren.next();
for (; !step.done; step = newChildren.next()) {
const child = step.value;
knownKeys = warnOnDuplicateKey(child, knownKeys);
}
}
}

const newChildren = iteratorFn.call(newChildrenIterable);
if (newChildren == null) {
throw new Error('An iterable object provided no iterator.');
}
invariant(
newChildren != null,
'An iterable object provided no iterator.',
);

let resultingFirstChild : ?Fiber = null;
let previousNewFiber : ?Fiber = null;
Expand Down
54 changes: 36 additions & 18 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,18 @@ var {
} = require('ReactTypeOfSideEffect');
var ReactCurrentOwner = require('ReactCurrentOwner');
var ReactFiberClassComponent = require('ReactFiberClassComponent');
var warning = require('warning');
var invariant = require('invariant');

if (__DEV__) {
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
var warning = require('warning');

var warnedAboutStatelessRefs = {};
}

const internalErrorMessage =
'This error is likely caused by a bug in React. Please file an issue.';

module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config : HostConfig<T, P, I, TI, PI, C, CX, PL>,
hostContext : HostContext<C, CX>,
Expand Down Expand Up @@ -341,9 +345,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// we don't do the bailout and we have to reuse existing props instead.
if (nextProps === null) {
nextProps = memoizedProps;
if (!nextProps) {
throw new Error('We should always have pending or current props.');
}
invariant(
nextProps !== null,
'We should always have pending or current props. (%s)',
internalErrorMessage
);
}
} else if (nextProps === null || memoizedProps === nextProps) {
if (memoizedProps.hidden &&
Expand Down Expand Up @@ -442,9 +448,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
}

function mountIndeterminateComponent(current, workInProgress, priorityLevel) {
if (current) {
throw new Error('An indeterminate component should never have mounted.');
}
invariant(
current === null,
'An indeterminate component should never have mounted. (%s)',
internalErrorMessage
);
var fn = workInProgress.type;
var props = workInProgress.pendingProps;
var unmaskedContext = getUnmaskedContext(workInProgress);
Expand Down Expand Up @@ -511,9 +519,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// we don't do the bailout and we have to reuse existing props instead.
if (nextCoroutine === null) {
nextCoroutine = current && current.memoizedProps;
if (!nextCoroutine) {
throw new Error('We should always have pending or current props.');
}
invariant(
nextCoroutine != null,
'We should always have pending or current props. (%s)',
internalErrorMessage
);
}
} else if (nextCoroutine === null || workInProgress.memoizedProps === nextCoroutine) {
nextCoroutine = workInProgress.memoizedProps;
Expand Down Expand Up @@ -575,9 +585,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// we don't do the bailout and we have to reuse existing props instead.
if (nextChildren === null) {
nextChildren = current && current.memoizedProps;
if (!nextChildren) {
throw new Error('We should always have pending or current props.');
}
invariant(
nextChildren != null,
'We should always have pending or current props. (%s)',
internalErrorMessage
);
}
} else if (nextChildren === null || workInProgress.memoizedProps === nextChildren) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
Expand Down Expand Up @@ -727,15 +739,21 @@ 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. (%s)',
internalErrorMessage
);
}
}

function beginFailedWork(current : ?Fiber, workInProgress : Fiber, priorityLevel : PriorityLevel) {
if (workInProgress.tag !== ClassComponent &&
workInProgress.tag !== HostRoot) {
throw new Error('Invalid type of work');
}
invariant(
workInProgress.tag === ClassComponent ||
workInProgress.tag === HostRoot,
'Invalid type of work. (%s)',
internalErrorMessage
);

// Add an error effect so we can handle the error during the commit phase
workInProgress.effectTag |= Err;
Expand Down
27 changes: 18 additions & 9 deletions src/renderers/shared/fiber/ReactFiberClassComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ var invariant = require('invariant');

const isArray = Array.isArray;

const internalErrorMessage =
'This error is likely caused by a bug in React. Please file an issue.';

module.exports = function(
scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void,
getPriorityContext : () => PriorityLevel,
Expand Down Expand Up @@ -257,9 +260,11 @@ module.exports = function(
const state = instance.state || null;

let props = workInProgress.pendingProps;
if (!props) {
throw new Error('There must be pending props for an initial mount.');
}
invariant(
props,
'There must be pending props for an initial mount. (%s)',
internalErrorMessage
);

const unmaskedContext = getUnmaskedContext(workInProgress);

Expand Down Expand Up @@ -298,9 +303,11 @@ module.exports = function(
// If there isn't any new props, then we'll reuse the memoized props.
// This could be from already completed work.
newProps = workInProgress.memoizedProps;
if (!newProps) {
throw new Error('There should always be pending or memoized props.');
}
invariant(
newProps != null,
'There should always be pending or memoized props. (%s)',
internalErrorMessage
);
}
const newUnmaskedContext = getUnmaskedContext(workInProgress);
const newContext = getMaskedContext(workInProgress, newUnmaskedContext);
Expand Down Expand Up @@ -363,9 +370,11 @@ module.exports = function(
// If there aren't any new props, then we'll reuse the memoized props.
// This could be from already completed work.
newProps = oldProps;
if (!newProps) {
throw new Error('There should always be pending or memoized props.');
}
invariant(
newProps != null,
'There should always be pending or memoized props. (%s)',
internalErrorMessage
);
}
const oldContext = instance.context;
const newUnmaskedContext = getUnmaskedContext(workInProgress);
Expand Down
49 changes: 39 additions & 10 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ var {
ContentReset,
} = require('ReactTypeOfSideEffect');

var invariant = require('invariant');

const internalErrorMessage =
'This error is likely caused by a bug in React. Please file an issue.';

module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config : HostConfig<T, P, I, TI, PI, C, CX, PL>,
captureError : (failedFiber : Fiber, error: Error) => ?Fiber
Expand Down Expand Up @@ -94,7 +99,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
}
parent = parent.return;
}
throw new Error('Expected to find a host parent.');
invariant(
false,
'Expected to find a host parent. (%s)',
internalErrorMessage
);
}

function getHostParentFiber(fiber : Fiber) : Fiber {
Expand All @@ -105,7 +114,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
}
parent = parent.return;
}
throw new Error('Expected to find a host parent.');
invariant(
false,
'Expected to find a host parent. (%s)',
internalErrorMessage
);
}

function isHostParent(fiber : Fiber) : boolean {
Expand Down Expand Up @@ -173,7 +186,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
parent = parentFiber.stateNode.containerInfo;
break;
default:
throw new Error('Invalid host parent fiber.');
invariant(
false,
'Invalid host parent fiber. (%s)',
internalErrorMessage
);
}
if (parentFiber.effectTag & ContentReset) {
// Reset the text content of the parent before doing any insertions
Expand Down Expand Up @@ -374,9 +391,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
return;
}
case HostText: {
if (finishedWork.stateNode == null || !current) {
throw new Error('This should only be done during updates.');
}
invariant(
finishedWork.stateNode !== null && current != null,
'This should only be done during updates. (%s)',
internalErrorMessage
);
const textInstance : TI = finishedWork.stateNode;
const newText : string = finishedWork.memoizedProps;
const oldText : string = current.memoizedProps;
Expand All @@ -389,8 +408,13 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
case HostPortal: {
return;
}
default:
throw new Error('This unit of work tag should not have side-effects.');
default: {
invariant(
false,
'This unit of work tag should not have side-effects. (%s)',
internalErrorMessage
);
}
}
}

Expand Down Expand Up @@ -450,8 +474,13 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// We have no life-cycles associated with portals.
return;
}
default:
throw new Error('This unit of work tag should not have side-effects.');
default: {
invariant(
false,
'This unit of work tag should not have side-effects. (%s)',
internalErrorMessage
);
}
}
}

Expand Down
Loading