Skip to content
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.

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

Re-enable Flow for ReactFiber and fix Flow issues #12842

Merged
merged 4 commits into from
May 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .flowconfig
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
./node_modules/fbjs/flow/lib/dev.js
./scripts/flow

[lints]
untyped-type-import=error

[options]
esproposal.class_static_fields=enable
esproposal.class_instance_fields=enable
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/events/ReactDOMEventListener.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function getTopLevelCallbackBookKeeping(
): {
topLevelType: ?DOMTopLevelEventType,
nativeEvent: ?AnyNativeEvent,
targetInst: Fiber,
targetInst: Fiber | null,
ancestors: Array<Fiber>,
} {
if (callbackBookkeepingPool.length) {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactCapturedValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export type CapturedError = {

export function createCapturedValue<T>(
value: T,
source: Fiber | null,
source: Fiber,
): CapturedValue<T> {
// If the value is an error, call this function immediately after it is thrown
// so the stack is accurate.
Expand Down
108 changes: 45 additions & 63 deletions packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import type {ReactElement, Source} from 'shared/ReactElementType';
import type {ReactPortal, RefObject} from 'shared/ReactTypes';
import type {ReactFragment, ReactPortal, RefObject} from 'shared/ReactTypes';
import type {TypeOfWork} from 'shared/ReactTypeOfWork';
import type {TypeOfMode} from './ReactTypeOfMode';
import type {TypeOfSideEffect} from 'shared/ReactTypeOfSideEffect';
Expand Down Expand Up @@ -316,7 +317,7 @@ export function createWorkInProgress(
return workInProgress;
}

export function createHostRootFiber(isAsync): Fiber {
export function createHostRootFiber(isAsync: boolean): Fiber {
const mode = isAsync ? AsyncMode | StrictMode : NoContext;
return createFiber(HostRoot, null, null, mode);
}
Expand Down Expand Up @@ -366,42 +367,9 @@ export function createFiberFromElement(
// mode compatible.
mode |= StrictMode;
break;
default: {
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 had to extract this because Flow couldn't follow that fiberTag is always assigned in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I was wondering about this bit.

if (typeof type === 'object' && type !== null) {
switch (type.$$typeof) {
case REACT_PROVIDER_TYPE:
fiberTag = ContextProvider;
break;
case REACT_CONTEXT_TYPE:
// This is a consumer
fiberTag = ContextConsumer;
break;
case REACT_FORWARD_REF_TYPE:
fiberTag = ForwardRef;
break;
default:
if (typeof type.tag === 'number') {
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 cut this part out completely because it was only relevant to call/return.

// Currently assumed to be a continuation and therefore is a
// fiber already.
// TODO: The yield system is currently broken for updates in
// some cases. The reified yield stores a fiber, but we don't
// know which fiber that is; the current or a workInProgress?
// When the continuation gets rendered here we don't know if we
// can reuse that fiber or if we need to clone it. There is
// probably a clever way to restructure this.
fiber = ((type: any): Fiber);
fiber.pendingProps = pendingProps;
fiber.expirationTime = expirationTime;
return fiber;
} else {
throwOnInvalidElementType(type, owner);
}
break;
}
} else {
throwOnInvalidElementType(type, owner);
}
}
default:
fiberTag = getFiberTagFromObjectType(type, owner);
break;
}
}

Expand All @@ -417,33 +385,47 @@ export function createFiberFromElement(
return fiber;
}

function throwOnInvalidElementType(type, owner) {
let info = '';
if (__DEV__) {
if (
type === undefined ||
(typeof type === 'object' &&
type !== null &&
Object.keys(type).length === 0)
) {
info +=
' You likely forgot to export your component from the file ' +
"it's defined in, or you might have mixed up default and " +
'named imports.';
}
const ownerName = owner ? getComponentName(owner) : null;
if (ownerName) {
info += '\n\nCheck the render method of `' + ownerName + '`.';
function getFiberTagFromObjectType(type, owner): TypeOfWork {
const $$typeof =
typeof type === 'object' && type !== null ? type.$$typeof : null;

switch ($$typeof) {
case REACT_PROVIDER_TYPE:
return ContextProvider;
case REACT_CONTEXT_TYPE:
// This is a consumer
return ContextConsumer;
case REACT_FORWARD_REF_TYPE:
return ForwardRef;
default: {
let info = '';
if (__DEV__) {
if (
type === undefined ||
(typeof type === 'object' &&
type !== null &&
Object.keys(type).length === 0)
) {
info +=
' You likely forgot to export your component from the file ' +
"it's defined in, or you might have mixed up default and " +
'named imports.';
}
const ownerName = owner ? getComponentName(owner) : null;
if (ownerName) {
info += '\n\nCheck the render method of `' + ownerName + '`.';
}
}
invariant(
false,
'Element type is invalid: expected a string (for built-in ' +
'components) or a class/function (for composite components) ' +
'but got: %s.%s',
type == null ? type : typeof type,
info,
);
}
}
invariant(
false,
'Element type is invalid: expected a string (for built-in ' +
'components) or a class/function (for composite components) ' +
'but got: %s.%s',
type == null ? type : typeof type,
info,
);
}

export function createFiberFromFragment(
Expand Down
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import type {HostConfig} from 'react-reconciler';
import type {Fiber} from './ReactFiber';
import type {FiberRoot} from './ReactFiber';
import type {FiberRoot} from './ReactFiberRoot';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {CapturedValue, CapturedError} from './ReactCapturedValue';

Expand Down Expand Up @@ -59,7 +59,7 @@ if (__DEV__) {
export function logError(boundary: Fiber, errorInfo: CapturedValue<mixed>) {
const source = errorInfo.source;
let stack = errorInfo.stack;
if (stack === null) {
if (stack === null && source !== null) {
stack = getStackAddendumByWorkInProgressFiber(source);
}

Expand Down Expand Up @@ -94,7 +94,7 @@ export function logError(boundary: Fiber, errorInfo: CapturedValue<mixed>) {

export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
config: HostConfig<T, P, I, TI, HI, PI, C, CC, CX, PL>,
captureError: (failedFiber: Fiber, error: mixed) => Fiber | null,
captureError: (failedFiber: Fiber, error: mixed) => void,
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 checked return value isn't being used. This is a type that wasn't updated.

scheduleWork: (
fiber: Fiber,
startTime: ExpirationTime,
Expand Down
85 changes: 52 additions & 33 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,10 +321,20 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
}

// Restore the original state of the work-in-progress
if (stashedWorkInProgressProperties === null) {
// This should never happen. Don't throw because this code is DEV-only.
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 shouldn't happen in practice because we always assign them before performing work.
If it does we'll want to know.

warning(
false,
'Could not replay rendering after an error. This is likely a bug in React. ' +
'Please file an issue.',
);
return;
}
assignFiberPropertiesInDEV(
failedUnitOfWork,
stashedWorkInProgressProperties,
);

switch (failedUnitOfWork.tag) {
case HostRoot:
popHostContainer(failedUnitOfWork);
Expand Down Expand Up @@ -1073,43 +1083,52 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
// This is a fatal error.
didFatal = true;
onUncaughtError(thrownValue);
break;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Flow wasn't sure nextUnitOfWork isn't null without an explicit if / else here.
There's another break below the whole thing.

}
} else {
if (__DEV__) {
// Reset global debug state
// We assume this is defined in DEV
(resetCurrentlyProcessingQueue: any)();
}

if (__DEV__) {
// Reset global debug state
// We assume this is defined in DEV
(resetCurrentlyProcessingQueue: any)();
}
const failedUnitOfWork: Fiber = nextUnitOfWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
replayUnitOfWork(failedUnitOfWork, thrownValue, isAsync);
}

if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
const failedUnitOfWork = nextUnitOfWork;
replayUnitOfWork(failedUnitOfWork, thrownValue, isAsync);
}
// TODO: we already know this isn't true in some cases.
// At least this shows a nicer error message until we figure out the cause.
// https://github.com/facebook/react/issues/12449#issuecomment-386727431
invariant(
nextUnitOfWork !== null,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an invariant in DEV-only code. But we know it already crashes (#12449 (comment)).
Maybe @acdlite can think of a better fix but for now I'm making the existing bug explicit in the hope that somebody gives us a repro case.

'Failed to replay rendering after an error. This ' +
'is likely caused by a bug in React. Please file an issue ' +
'with a reproducing case to help us find it.',
);

const sourceFiber: Fiber = nextUnitOfWork;
let returnFiber = sourceFiber.return;
if (returnFiber === null) {
// This is the root. The root could capture its own errors. However,
// we don't know if it errors before or after we pushed the host
// context. This information is needed to avoid a stack mismatch.
// Because we're not sure, treat this as a fatal error. We could track
// which phase it fails in, but doesn't seem worth it. At least
// for now.
didFatal = true;
onUncaughtError(thrownValue);
break;
const sourceFiber: Fiber = nextUnitOfWork;
let returnFiber = sourceFiber.return;
if (returnFiber === null) {
// This is the root. The root could capture its own errors. However,
// we don't know if it errors before or after we pushed the host
// context. This information is needed to avoid a stack mismatch.
// Because we're not sure, treat this as a fatal error. We could track
// which phase it fails in, but doesn't seem worth it. At least
// for now.
didFatal = true;
onUncaughtError(thrownValue);
break;
}
throwException(
root,
returnFiber,
sourceFiber,
thrownValue,
nextRenderIsExpired,
nextRenderExpirationTime,
mostRecentCurrentTimeMs,
);
nextUnitOfWork = completeUnitOfWork(sourceFiber);
}
throwException(
root,
returnFiber,
sourceFiber,
thrownValue,
nextRenderIsExpired,
nextRenderExpirationTime,
mostRecentCurrentTimeMs,
);
nextUnitOfWork = completeUnitOfWork(sourceFiber);
}
break;
} while (true);
Expand Down
22 changes: 15 additions & 7 deletions packages/react-reconciler/src/ReactStrictModeWarnings.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,15 @@ if (__DEV__) {
pendingUnsafeLifecycleWarnings = new Map();
};

const getStrictRoot = (fiber: Fiber): Fiber => {
const findStrictRoot = (fiber: Fiber): Fiber | null => {
let maybeStrictRoot = null;

while (fiber !== null) {
if (fiber.mode & StrictMode) {
maybeStrictRoot = fiber;
let node = fiber;
while (node !== null) {
if (node.mode & StrictMode) {
maybeStrictRoot = node;
}

fiber = fiber.return;
node = node.return;
}

return maybeStrictRoot;
Expand Down Expand Up @@ -225,7 +225,15 @@ if (__DEV__) {
fiber: Fiber,
instance: any,
) => {
const strictRoot = getStrictRoot(fiber);
const strictRoot = findStrictRoot(fiber);
if (strictRoot === null) {
warning(
false,
'Expected to find a StrictMode component in a strict mode tree. ' +
'This error is likely caused by a bug in React. Please file an issue.',
);
return;
}

// Dedup strategy: Warn once per component.
// This is difficult to track any other way since component names
Expand Down