Skip to content

Commit

Permalink
Add missing null checks to OffscreenInstance code
Browse files Browse the repository at this point in the history
`stateNode` is any-typed, so when reading from `stateNode` we should always cast
it to the specific type for that type of work. I noticed a place in the commit
phase where OffscreenInstance wasn't being cast. When I added the type
assertion, it exposed some type errors where nullable values were being accessed
without first being refined.

I added the required null checks without verifying the logic of the existing
code. If the existing logic was correct, then the extra null checks won't have
any affect on the behavior, because all they do is refine from a nullable type
to a non-nullable type in places where the type was assumed to already be
non-nullable. But the result looks a bit fishy to me, so I also left behind some
TODOs to follow up and verify it's correct.
  • Loading branch information
acdlite committed Jul 5, 2022
1 parent 4cd788a commit 6d1e570
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 16 deletions.
31 changes: 24 additions & 7 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import type {Wakeable} from 'shared/ReactTypes';
import type {
OffscreenState,
OffscreenInstance,
OffscreenQueue,
} from './ReactFiberOffscreenComponent';
import type {HookFlags} from './ReactHookEffectTags';
import type {Cache} from './ReactFiberCacheComponent.new';
Expand Down Expand Up @@ -2877,8 +2878,8 @@ function commitPassiveMountOnFiber(

if (enableTransitionTracing) {
const isFallback = finishedWork.memoizedState;
const queue = (finishedWork.updateQueue: any);
const instance = finishedWork.stateNode;
const queue: OffscreenQueue = (finishedWork.updateQueue: any);
const instance: OffscreenInstance = finishedWork.stateNode;

if (queue !== null) {
if (isFallback) {
Expand All @@ -2896,7 +2897,11 @@ function commitPassiveMountOnFiber(
// Add all the transitions saved in the update queue during
// the render phase (ie the transitions associated with this boundary)
// into the transitions set.
prevTransitions.add(transition);
if (prevTransitions === null) {
// TODO: What if prevTransitions is null?
} else {
prevTransitions.add(transition);
}
});
}

Expand All @@ -2913,10 +2918,22 @@ function commitPassiveMountOnFiber(
// caused them
if (markerTransitions !== null) {
markerTransitions.forEach(transition => {
if (instance.transitions.has(transition)) {
instance.pendingMarkers.add(
markerInstance.pendingSuspenseBoundaries,
);
if (instance.transitions === null) {
// TODO: What if instance.transitions is null?
} else {
if (instance.transitions.has(transition)) {
if (
instance.pendingMarkers === null ||
markerInstance.pendingSuspenseBoundaries === null
) {
// TODO: What if instance.pendingMarkers is null?
// TODO: What if markerInstance.pendingSuspenseBoundaries is null?
} else {
instance.pendingMarkers.add(
markerInstance.pendingSuspenseBoundaries,
);
}
}
}
});
}
Expand Down
31 changes: 24 additions & 7 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import type {Wakeable} from 'shared/ReactTypes';
import type {
OffscreenState,
OffscreenInstance,
OffscreenQueue,
} from './ReactFiberOffscreenComponent';
import type {HookFlags} from './ReactHookEffectTags';
import type {Cache} from './ReactFiberCacheComponent.old';
Expand Down Expand Up @@ -2877,8 +2878,8 @@ function commitPassiveMountOnFiber(

if (enableTransitionTracing) {
const isFallback = finishedWork.memoizedState;
const queue = (finishedWork.updateQueue: any);
const instance = finishedWork.stateNode;
const queue: OffscreenQueue = (finishedWork.updateQueue: any);
const instance: OffscreenInstance = finishedWork.stateNode;

if (queue !== null) {
if (isFallback) {
Expand All @@ -2896,7 +2897,11 @@ function commitPassiveMountOnFiber(
// Add all the transitions saved in the update queue during
// the render phase (ie the transitions associated with this boundary)
// into the transitions set.
prevTransitions.add(transition);
if (prevTransitions === null) {
// TODO: What if prevTransitions is null?
} else {
prevTransitions.add(transition);
}
});
}

Expand All @@ -2913,10 +2918,22 @@ function commitPassiveMountOnFiber(
// caused them
if (markerTransitions !== null) {
markerTransitions.forEach(transition => {
if (instance.transitions.has(transition)) {
instance.pendingMarkers.add(
markerInstance.pendingSuspenseBoundaries,
);
if (instance.transitions === null) {
// TODO: What if instance.transitions is null?
} else {
if (instance.transitions.has(transition)) {
if (
instance.pendingMarkers === null ||
markerInstance.pendingSuspenseBoundaries === null
) {
// TODO: What if instance.pendingMarkers is null?
// TODO: What if markerInstance.pendingSuspenseBoundaries is null?
} else {
instance.pendingMarkers.add(
markerInstance.pendingSuspenseBoundaries,
);
}
}
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export type BatchConfigTransition = {
export type TracingMarkerInstance = {|
pendingSuspenseBoundaries: PendingSuspenseBoundaries | null,
transitions: Set<Transition> | null,
|} | null;
|};

export type PendingSuspenseBoundaries = Map<OffscreenInstance, SuspenseInfo>;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export type BatchConfigTransition = {
export type TracingMarkerInstance = {|
pendingSuspenseBoundaries: PendingSuspenseBoundaries | null,
transitions: Set<Transition> | null,
|} | null;
|};

export type PendingSuspenseBoundaries = Map<OffscreenInstance, SuspenseInfo>;

Expand Down

0 comments on commit 6d1e570

Please sign in to comment.