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

Jump to bottom

Bugfix: Nested useOpaqueIdentifier references #22553

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Bugfix: Nested useOpaqueIdentifier references #22553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c71d3ab
Handle render phase updates explicitly
acdlite Oct 13, 2021
aca9278
Bugfix: Nested useOpaqueIdentifier calls
acdlite Oct 13, 2021
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
35 changes: 35 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1975,5 +1975,40 @@ describe('ReactDOMServerHooks', () => {
container.getElementsByTagName('span')[0].getAttribute('id'),
).not.toBeNull();
});

it('useOpaqueIdentifier with multiple ids in nested components', async () => {
function DivWithId({id, children}) {
return <div id={id}>{children}</div>;
}

let setShowMore;
function App() {
const outerId = useOpaqueIdentifier();
const innerId = useOpaqueIdentifier();
const [showMore, _setShowMore] = useState(false);
setShowMore = _setShowMore;
return showMore ? (
<DivWithId id={outerId}>
<DivWithId id={innerId} />
</DivWithId>
) : null;
}

const container = document.createElement('div');
container.innerHTML = ReactDOMServer.renderToString(<App />);

await act(async () => {
ReactDOM.hydrateRoot(container, <App />);
});

// Show additional content that wasn't part of the initial server-
// rendered repsonse.
await act(async () => {
setShowMore(true);
});
const [div1, div2] = container.getElementsByTagName('div');
expect(typeof div1.getAttribute('id')).toBe('string');
expect(typeof div2.getAttribute('id')).toBe('string');
});
});
});
193 changes: 110 additions & 83 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes;
// includes unprocessed updates, not work in bailed out children.
let workInProgressRootSkippedLanes: Lanes = NoLanes;
// Lanes that were updated (in an interleaved event) during this render.
let workInProgressRootUpdatedLanes: Lanes = NoLanes;
let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
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 split workInProgressRootUpdatedLanes into two separate variables that track "interleaved" updates (ones that originate from a regular DOM event, when React has yielded to the main thread) versus render phase updates (ones that originate from within a rendering component).

// Lanes that were updated during the render phase (*not* an interleaved event).
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
// Lanes that were pinged (in an interleaved event) during this render.
let workInProgressRootPingedLanes: Lanes = NoLanes;

Expand Down Expand Up @@ -454,86 +456,105 @@ export function scheduleUpdateOnFiber(
eventTime: number,
): FiberRoot | null {
checkForNestedUpdates();
warnAboutRenderPhaseUpdatesInDEV(fiber);

const root = markUpdateLaneFromFiberToRoot(fiber, lane);
if (root === null) {
return null;
}

if (enableUpdaterTracking) {
if (isDevToolsPresent) {
addFiberToLanesMap(root, fiber, lane);
}
}

// Mark that the root has a pending update.
markRootUpdated(root, lane, eventTime);

if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
if (
(executionContext & CommitContext) !== NoContext &&
root === rootCommittingMutationOrLayoutEffects
) {
if (fiber.mode & ProfileMode) {
let current = fiber;
while (current !== null) {
if (current.tag === Profiler) {
const {id, onNestedUpdateScheduled} = current.memoizedProps;
if (typeof onNestedUpdateScheduled === 'function') {
onNestedUpdateScheduled(id);
if (
(executionContext & RenderContext) !== NoLanes &&
root === workInProgressRoot
) {
Copy link
Collaborator Author

@acdlite acdlite Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest reviewing this block with whitespace changes hidden; only a few lines changed: c71d3ab?diff=split&w=1

// This update was dispatched during the render phase. This is a mistake
// if the update originates from user space (with the exception of local
// hook updates, which are handled differently and don't reach this
// function), but there are some internal React features that use this as
// an implementation detail, like selective hydration
// and useOpaqueIdentifier.
warnAboutRenderPhaseUpdatesInDEV(fiber);

// Track lanes that were updated during the render phase
workInProgressRootRenderPhaseUpdatedLanes = mergeLanes(
workInProgressRootRenderPhaseUpdatedLanes,
lane,
);
} else {
// This is a normal update, scheduled from outside the render phase. For
// example, during an input event.
if (enableUpdaterTracking) {
if (isDevToolsPresent) {
addFiberToLanesMap(root, fiber, lane);
}
}

if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
if (
(executionContext & CommitContext) !== NoContext &&
root === rootCommittingMutationOrLayoutEffects
) {
if (fiber.mode & ProfileMode) {
let current = fiber;
while (current !== null) {
if (current.tag === Profiler) {
const {id, onNestedUpdateScheduled} = current.memoizedProps;
if (typeof onNestedUpdateScheduled === 'function') {
onNestedUpdateScheduled(id);
}
}
current = current.return;
}
current = current.return;
}
}
}
}

// TODO: Consolidate with `isInterleavedUpdate` check
if (root === workInProgressRoot) {
// Received an update to a tree that's in the middle of rendering. Mark
// that there was an interleaved update work on this root. Unless the
// `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render
// phase update. In that case, we don't treat render phase updates as if
// they were interleaved, for backwards compat reasons.
// TODO: Consolidate with `isInterleavedUpdate` check
if (root === workInProgressRoot) {
// Received an update to a tree that's in the middle of rendering. Mark
// that there was an interleaved update work on this root. Unless the
// `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render
// phase update. In that case, we don't treat render phase updates as if
// they were interleaved, for backwards compat reasons.
if (
deferRenderPhaseUpdateToNextBatch ||
(executionContext & RenderContext) === NoContext
) {
workInProgressRootInterleavedUpdatedLanes = mergeLanes(
workInProgressRootInterleavedUpdatedLanes,
lane,
);
}
if (workInProgressRootExitStatus === RootSuspendedWithDelay) {
// The root already suspended with a delay, which means this render
// definitely won't finish. Since we have a new update, let's mark it as
// suspended now, right before marking the incoming update. This has the
// effect of interrupting the current render and switching to the update.
// TODO: Make sure this doesn't override pings that happen while we've
// already started rendering.
markRootSuspended(root, workInProgressRootRenderLanes);
}
}

ensureRootIsScheduled(root, eventTime);
if (
deferRenderPhaseUpdateToNextBatch ||
(executionContext & RenderContext) === NoContext
lane === SyncLane &&
executionContext === NoContext &&
(fiber.mode & ConcurrentMode) === NoMode &&
// Treat `act` as if it's inside `batchedUpdates`, even in legacy mode.
!(__DEV__ && ReactCurrentActQueue.isBatchingLegacy)
) {
workInProgressRootUpdatedLanes = mergeLanes(
workInProgressRootUpdatedLanes,
lane,
);
}
if (workInProgressRootExitStatus === RootSuspendedWithDelay) {
// The root already suspended with a delay, which means this render
// definitely won't finish. Since we have a new update, let's mark it as
// suspended now, right before marking the incoming update. This has the
// effect of interrupting the current render and switching to the update.
// TODO: Make sure this doesn't override pings that happen while we've
// already started rendering.
markRootSuspended(root, workInProgressRootRenderLanes);
// Flush the synchronous work now, unless we're already working or inside
// a batch. This is intentionally inside scheduleUpdateOnFiber instead of
// scheduleCallbackForFiber to preserve the ability to schedule a callback
// without immediately flushing it. We only do this for user-initiated
// updates, to preserve historical behavior of legacy mode.
resetRenderTimer();
flushSyncCallbacksOnlyInLegacyMode();
}
}

ensureRootIsScheduled(root, eventTime);
if (
lane === SyncLane &&
executionContext === NoContext &&
(fiber.mode & ConcurrentMode) === NoMode &&
// Treat `act` as if it's inside `batchedUpdates`, even in legacy mode.
!(__DEV__ && ReactCurrentActQueue.isBatchingLegacy)
) {
// Flush the synchronous work now, unless we're already working or inside
// a batch. This is intentionally inside scheduleUpdateOnFiber instead of
// scheduleCallbackForFiber to preserve the ability to schedule a callback
// without immediately flushing it. We only do this for user-initiated
// updates, to preserve historical behavior of legacy mode.
resetRenderTimer();
flushSyncCallbacksOnlyInLegacyMode();
}

return root;
}

Expand Down Expand Up @@ -865,7 +886,25 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
clearContainer(root.containerInfo);
}

const exitStatus = renderRootSync(root, errorRetryLanes);
let exitStatus;

const MAX_ERROR_RETRY_ATTEMPTS = 50;
for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) {
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 the key part to review

exitStatus = renderRootSync(root, errorRetryLanes);
if (
exitStatus === RootErrored &&
workInProgressRootRenderPhaseUpdatedLanes !== NoLanes
) {
// There was a render phase update during this render. This was likely a
// useOpaqueIdentifier hook upgrading itself to a client ID. Try rendering
// again. This time, the component will use a client ID and will proceed
// without throwing. If multiple IDs upgrade as a result of the same
// update, we will have to do multiple render passes. To protect against
// an inifinite loop, eventually we'll give up.
continue;
}
break;
}

executionContext = prevExecutionContext;

Expand Down Expand Up @@ -1042,7 +1081,10 @@ function markRootSuspended(root, suspendedLanes) {
// TODO: Lol maybe there's a better way to factor this besides this
// obnoxiously named function :)
suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);
suspendedLanes = removeLanes(suspendedLanes, workInProgressRootUpdatedLanes);
suspendedLanes = removeLanes(
suspendedLanes,
workInProgressRootInterleavedUpdatedLanes,
);
markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);
}

Expand All @@ -1068,30 +1110,15 @@ function performSyncWorkOnRoot(root) {

let exitStatus = renderRootSync(root, lanes);
if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
const prevExecutionContext = executionContext;
executionContext |= RetryAfterError;

// If an error occurred during hydration,
// discard server response and fall back to client side render.
if (root.isDehydrated) {
root.isDehydrated = false;
if (__DEV__) {
errorHydratingContainer(root.containerInfo);
}
clearContainer(root.containerInfo);
}

// If something threw an error, try rendering one more time. We'll render
// synchronously to block concurrent data mutations, and we'll includes
// all pending updates are included. If it still fails after the second
// attempt, we'll give up and commit the resulting tree.
const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root);
if (errorRetryLanes !== NoLanes) {
lanes = errorRetryLanes;
exitStatus = renderRootSync(root, lanes);
exitStatus = recoverFromConcurrentError(root, errorRetryLanes);
}

executionContext = prevExecutionContext;
}

if (exitStatus === RootFatalErrored) {
Expand Down Expand Up @@ -1300,7 +1327,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootExitStatus = RootIncomplete;
workInProgressRootFatalError = null;
workInProgressRootSkippedLanes = NoLanes;
workInProgressRootUpdatedLanes = NoLanes;
workInProgressRootInterleavedUpdatedLanes = NoLanes;
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;

enqueueInterleavedUpdates();
Expand Down Expand Up @@ -1443,7 +1471,7 @@ export function renderDidSuspendDelayIfPossible(): void {
if (
workInProgressRoot !== null &&
(includesNonIdleWork(workInProgressRootSkippedLanes) ||
includesNonIdleWork(workInProgressRootUpdatedLanes))
includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes))
) {
// Mark the current render as suspended so that we switch to working on
// the updates that were skipped. Usually we only suspend at the end of
Expand Down Expand Up @@ -2697,7 +2725,6 @@ function warnAboutRenderPhaseUpdatesInDEV(fiber) {
if (__DEV__) {
if (
ReactCurrentDebugFiberIsRenderingInDEV &&
(executionContext & RenderContext) !== NoContext &&
!getIsUpdatingOpaqueValueInRenderPhaseInDEV()
) {
switch (fiber.tag) {
Expand Down
Loading