Skip to content

Commit c1f5884

Browse files
authored
Add missing null checks to OffscreenInstance code (#24846)
`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.
1 parent 4cd788a commit c1f5884

File tree

4 files changed

+50
-16
lines changed

4 files changed

+50
-16
lines changed

packages/react-reconciler/src/ReactFiberCommitWork.new.js

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import type {Wakeable} from 'shared/ReactTypes';
2525
import type {
2626
OffscreenState,
2727
OffscreenInstance,
28+
OffscreenQueue,
2829
} from './ReactFiberOffscreenComponent';
2930
import type {HookFlags} from './ReactHookEffectTags';
3031
import type {Cache} from './ReactFiberCacheComponent.new';
@@ -2877,8 +2878,8 @@ function commitPassiveMountOnFiber(
28772878

28782879
if (enableTransitionTracing) {
28792880
const isFallback = finishedWork.memoizedState;
2880-
const queue = (finishedWork.updateQueue: any);
2881-
const instance = finishedWork.stateNode;
2881+
const queue: OffscreenQueue = (finishedWork.updateQueue: any);
2882+
const instance: OffscreenInstance = finishedWork.stateNode;
28822883

28832884
if (queue !== null) {
28842885
if (isFallback) {
@@ -2896,7 +2897,11 @@ function commitPassiveMountOnFiber(
28962897
// Add all the transitions saved in the update queue during
28972898
// the render phase (ie the transitions associated with this boundary)
28982899
// into the transitions set.
2899-
prevTransitions.add(transition);
2900+
if (prevTransitions === null) {
2901+
// TODO: What if prevTransitions is null?
2902+
} else {
2903+
prevTransitions.add(transition);
2904+
}
29002905
});
29012906
}
29022907

@@ -2913,10 +2918,22 @@ function commitPassiveMountOnFiber(
29132918
// caused them
29142919
if (markerTransitions !== null) {
29152920
markerTransitions.forEach(transition => {
2916-
if (instance.transitions.has(transition)) {
2917-
instance.pendingMarkers.add(
2918-
markerInstance.pendingSuspenseBoundaries,
2919-
);
2921+
if (instance.transitions === null) {
2922+
// TODO: What if instance.transitions is null?
2923+
} else {
2924+
if (instance.transitions.has(transition)) {
2925+
if (
2926+
instance.pendingMarkers === null ||
2927+
markerInstance.pendingSuspenseBoundaries === null
2928+
) {
2929+
// TODO: What if instance.pendingMarkers is null?
2930+
// TODO: What if markerInstance.pendingSuspenseBoundaries is null?
2931+
} else {
2932+
instance.pendingMarkers.add(
2933+
markerInstance.pendingSuspenseBoundaries,
2934+
);
2935+
}
2936+
}
29202937
}
29212938
});
29222939
}

packages/react-reconciler/src/ReactFiberCommitWork.old.js

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import type {Wakeable} from 'shared/ReactTypes';
2525
import type {
2626
OffscreenState,
2727
OffscreenInstance,
28+
OffscreenQueue,
2829
} from './ReactFiberOffscreenComponent';
2930
import type {HookFlags} from './ReactHookEffectTags';
3031
import type {Cache} from './ReactFiberCacheComponent.old';
@@ -2877,8 +2878,8 @@ function commitPassiveMountOnFiber(
28772878

28782879
if (enableTransitionTracing) {
28792880
const isFallback = finishedWork.memoizedState;
2880-
const queue = (finishedWork.updateQueue: any);
2881-
const instance = finishedWork.stateNode;
2881+
const queue: OffscreenQueue = (finishedWork.updateQueue: any);
2882+
const instance: OffscreenInstance = finishedWork.stateNode;
28822883

28832884
if (queue !== null) {
28842885
if (isFallback) {
@@ -2896,7 +2897,11 @@ function commitPassiveMountOnFiber(
28962897
// Add all the transitions saved in the update queue during
28972898
// the render phase (ie the transitions associated with this boundary)
28982899
// into the transitions set.
2899-
prevTransitions.add(transition);
2900+
if (prevTransitions === null) {
2901+
// TODO: What if prevTransitions is null?
2902+
} else {
2903+
prevTransitions.add(transition);
2904+
}
29002905
});
29012906
}
29022907

@@ -2913,10 +2918,22 @@ function commitPassiveMountOnFiber(
29132918
// caused them
29142919
if (markerTransitions !== null) {
29152920
markerTransitions.forEach(transition => {
2916-
if (instance.transitions.has(transition)) {
2917-
instance.pendingMarkers.add(
2918-
markerInstance.pendingSuspenseBoundaries,
2919-
);
2921+
if (instance.transitions === null) {
2922+
// TODO: What if instance.transitions is null?
2923+
} else {
2924+
if (instance.transitions.has(transition)) {
2925+
if (
2926+
instance.pendingMarkers === null ||
2927+
markerInstance.pendingSuspenseBoundaries === null
2928+
) {
2929+
// TODO: What if instance.pendingMarkers is null?
2930+
// TODO: What if markerInstance.pendingSuspenseBoundaries is null?
2931+
} else {
2932+
instance.pendingMarkers.add(
2933+
markerInstance.pendingSuspenseBoundaries,
2934+
);
2935+
}
2936+
}
29202937
}
29212938
});
29222939
}

packages/react-reconciler/src/ReactFiberTracingMarkerComponent.new.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export type BatchConfigTransition = {
4343
export type TracingMarkerInstance = {|
4444
pendingSuspenseBoundaries: PendingSuspenseBoundaries | null,
4545
transitions: Set<Transition> | null,
46-
|} | null;
46+
|};
4747

4848
export type PendingSuspenseBoundaries = Map<OffscreenInstance, SuspenseInfo>;
4949

packages/react-reconciler/src/ReactFiberTracingMarkerComponent.old.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export type BatchConfigTransition = {
4343
export type TracingMarkerInstance = {|
4444
pendingSuspenseBoundaries: PendingSuspenseBoundaries | null,
4545
transitions: Set<Transition> | null,
46-
|} | null;
46+
|};
4747

4848
export type PendingSuspenseBoundaries = Map<OffscreenInstance, SuspenseInfo>;
4949

0 commit comments

Comments
 (0)