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.
Andrew Clark committed
Jul 5, 2022 at 11:40 UTC
c1f5884ffeceb8be2277e10c81aeaffca2dfe9d8
4 files changed
+50
-16
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+24
-7
@@ -25,6 +25,7 @@ import type {Wakeable} from 'shared/ReactTypes';
25
import type {
26
OffscreenState,
27
OffscreenInstance,
28
+ OffscreenQueue,
29
} from './ReactFiberOffscreenComponent';
30
import type {HookFlags} from './ReactHookEffectTags';
31
import type {Cache} from './ReactFiberCacheComponent.new';
@@ -2877,8 +2878,8 @@ function commitPassiveMountOnFiber(
2878
2879
if (enableTransitionTracing) {
2880
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;
2883
2884
if (queue !== null) {
2885
if (isFallback) {
@@ -2896,7 +2897,11 @@ function commitPassiveMountOnFiber(
2897
// Add all the transitions saved in the update queue during
2898
// the render phase (ie the transitions associated with this boundary)
2899
// 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
+ }
2905
});
2906
}
2907
@@ -2913,10 +2918,22 @@ function commitPassiveMountOnFiber(
2918
// caused them
2919
if (markerTransitions !== null) {
2920
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
+ }
2937
}
2938
});
2939
}
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+24
-7
@@ -25,6 +25,7 @@ import type {Wakeable} from 'shared/ReactTypes';
25
import type {
26
OffscreenState,
27
OffscreenInstance,
28
+ OffscreenQueue,
29
} from './ReactFiberOffscreenComponent';
30
import type {HookFlags} from './ReactHookEffectTags';
31
import type {Cache} from './ReactFiberCacheComponent.old';
@@ -2877,8 +2878,8 @@ function commitPassiveMountOnFiber(
2878
2879
if (enableTransitionTracing) {
2880
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;
2883
2884
if (queue !== null) {
2885
if (isFallback) {
@@ -2896,7 +2897,11 @@ function commitPassiveMountOnFiber(
2897
// Add all the transitions saved in the update queue during
2898
// the render phase (ie the transitions associated with this boundary)
2899
// 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
+ }
2905
});
2906
}
2907
@@ -2913,10 +2918,22 @@ function commitPassiveMountOnFiber(
2918
// caused them
2919
if (markerTransitions !== null) {
2920
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
+ }
2937
}
2938
});
2939
}
packages/react-reconciler/src/ReactFiberTracingMarkerComponent.new.js
+1
-1
@@ -43,7 +43,7 @@ export type BatchConfigTransition = {
43
export type TracingMarkerInstance = {|
44
pendingSuspenseBoundaries: PendingSuspenseBoundaries | null,
45
transitions: Set<Transition> | null,
46
-|} | null;
46
+|};
47
48
export type PendingSuspenseBoundaries = Map<OffscreenInstance, SuspenseInfo>;
49
packages/react-reconciler/src/ReactFiberTracingMarkerComponent.old.js
+1
-1
@@ -43,7 +43,7 @@ export type BatchConfigTransition = {
43
export type TracingMarkerInstance = {|
44
pendingSuspenseBoundaries: PendingSuspenseBoundaries | null,
45
transitions: Set<Transition> | null,
46
-|} | null;
46
+|};
47
48
export type PendingSuspenseBoundaries = Map<OffscreenInstance, SuspenseInfo>;
49