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.

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

Set owner correctly inside forwardRef and context consumer #12777

Merged
merged 1 commit into from
May 14, 2018
Merged

Set owner correctly inside forwardRef and context consumer #12777

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 21 additions & 2 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,17 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
}
const nextChildren = render(nextProps, ref);

let nextChildren;
if (__DEV__) {
ReactCurrentOwner.current = workInProgress;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. We shouldn't set current owner, which is runtime behavior, only in DEV. This could mess up certain new features in prod for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

We were supposed to use the debug frame thing for DEV warnings instead of owner so that we didn't have to do this. Perhaps the fix is to change how the context warning works so that it doesn't rely on current owner? @acdlite

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh hm. for functional and indeterminate components it looks DEV-only:

react/packages/react-reconciler/src/ReactFiberBeginWork.js

Lines 260 to 344 in e8394da

ReactCurrentOwner.current = workInProgress;
ReactDebugCurrentFiber.setCurrentPhase('render');
nextChildren = fn(nextProps, context);
ReactDebugCurrentFiber.setCurrentPhase(null);
} else {
nextChildren = fn(nextProps, context);
}
// React DevTools reads this flag.
workInProgress.effectTag |= PerformedWork;
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextProps);
return workInProgress.child;
}
function updateClassComponent(
current: Fiber | null,
workInProgress: Fiber,
renderExpirationTime: ExpirationTime,
) {
// Push context providers early to prevent context stack mismatches.
// During mounting we don't know the child context yet as the instance doesn't exist.
// We will invalidate the child context in finishClassComponent() right after rendering.
const hasContext = pushLegacyContextProvider(workInProgress);
let shouldUpdate;
if (current === null) {
if (workInProgress.stateNode === null) {
// In the initial pass we might need to construct the instance.
constructClassInstance(
workInProgress,
workInProgress.pendingProps,
renderExpirationTime,
);
mountClassInstance(workInProgress, renderExpirationTime);
shouldUpdate = true;
} else {
// In a resume, we'll already have an instance we can reuse.
shouldUpdate = resumeMountClassInstance(
workInProgress,
renderExpirationTime,
);
}
} else {
shouldUpdate = updateClassInstance(
current,
workInProgress,
renderExpirationTime,
);
}
return finishClassComponent(
current,
workInProgress,
shouldUpdate,
hasContext,
renderExpirationTime,
);
}
function finishClassComponent(
current: Fiber | null,
workInProgress: Fiber,
shouldUpdate: boolean,
hasContext: boolean,
renderExpirationTime: ExpirationTime,
) {
// Refs should update even if shouldComponentUpdate returns false
markRef(current, workInProgress);
const didCaptureError =
(workInProgress.effectTag & DidCapture) !== NoEffect;
if (!shouldUpdate && !didCaptureError) {
// Context providers should defer to sCU for rendering
if (hasContext) {
invalidateContextProvider(workInProgress, false);
}
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
const ctor = workInProgress.type;
const instance = workInProgress.stateNode;
// Rerender
ReactCurrentOwner.current = workInProgress;

I guess that's an oversight?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think at the time this was intentional because we didn't separate those concepts, and didn't want to have the overhead of setting a property in PROD when it doesn't end up being used (functional components can't be ref owners which was the only runtime behavior of this feature).

Copy link
Contributor

Choose a reason for hiding this comment

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

We just have to be better at following through when we change strategies like introducing debug frame.

ReactDebugCurrentFiber.setCurrentPhase('render');
nextChildren = render(nextProps, ref);
ReactDebugCurrentFiber.setCurrentPhase(null);
} else {
nextChildren = render(nextProps, ref);
}

reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextProps);
return workInProgress.child;
Expand Down Expand Up @@ -1022,7 +1032,16 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
);
}

const newChildren = render(newValue);
let newChildren;
if (__DEV__) {
ReactCurrentOwner.current = workInProgress;
ReactDebugCurrentFiber.setCurrentPhase('render');
newChildren = render(newValue);
ReactDebugCurrentFiber.setCurrentPhase(null);
} else {
newChildren = render(newValue);
}

// React DevTools reads this flag.
workInProgress.effectTag |= PerformedWork;
reconcileChildren(current, workInProgress, newChildren);
Expand Down