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.

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

Check if suspensey instance resolves in immediate task #26427

Merged
merged 1 commit into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion packages/react-art/src/ReactARTHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -459,10 +459,15 @@ export function requestPostPaintCallback(callback: (time: number) => void) {
// noop
}

export function shouldSuspendCommit(type, props) {
export function maySuspendCommit(type, props) {
return false;
}

export function preloadInstance(type, props) {
// Return true to indicate it's already loaded
return true;
}

export function startSuspendingCommit() {}

export function suspendInstance(type, props) {}
Expand Down
7 changes: 6 additions & 1 deletion packages/react-dom-bindings/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -1609,10 +1609,15 @@ export function requestPostPaintCallback(callback: (time: number) => void) {
});
}

export function shouldSuspendCommit(type: Type, props: Props): boolean {
export function maySuspendCommit(type: Type, props: Props): boolean {
return false;
}

export function preloadInstance(type: Type, props: Props): boolean {
// Return true to indicate it's already loaded
return true;
}

export function startSuspendingCommit(): void {}

export function suspendInstance(type: Type, props: Props): void {}
Expand Down
6 changes: 5 additions & 1 deletion packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -414,10 +414,14 @@ export function requestPostPaintCallback(callback: (time: number) => void) {
// noop
}

export function shouldSuspendCommit(type: Type, props: Props): boolean {
export function maySuspendCommit(type: Type, props: Props): boolean {
return false;
}

export function preloadInstance(type: Type, props: Props): boolean {
return true;
}

export function startSuspendingCommit(): void {}

export function suspendInstance(type: Type, props: Props): void {}
Expand Down
7 changes: 6 additions & 1 deletion packages/react-native-renderer/src/ReactNativeHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -522,10 +522,15 @@ export function requestPostPaintCallback(callback: (time: number) => void) {
// noop
}

export function shouldSuspendCommit(type: Type, props: Props): boolean {
export function maySuspendCommit(type: Type, props: Props): boolean {
return false;
}

export function preloadInstance(type: Type, props: Props): boolean {
// Return true to indicate it's already loaded
return true;
}

export function startSuspendingCommit(): void {}

export function suspendInstance(type: Type, props: Props): void {}
Expand Down
81 changes: 43 additions & 38 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
if (record === undefined) {
throw new Error('Could not find record for key.');
}
if (record.status === 'pending') {
if (record.status === 'fulfilled') {
// Already loaded.
} else if (record.status === 'pending') {
if (suspenseyCommitSubscription === null) {
suspenseyCommitSubscription = {
pendingCount: 1,
Expand All @@ -321,20 +323,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
} else {
suspenseyCommitSubscription.pendingCount++;
}
// Stash the subscription on the record. In `resolveSuspenseyThing`,
// we'll use this fire the commit once all the things have loaded.
if (record.subscriptions === null) {
record.subscriptions = [];
}
record.subscriptions.push(suspenseyCommitSubscription);
}
// Stash the subscription on the record. In `resolveSuspenseyThing`,
// we'll use this fire the commit once all the things have loaded.
if (record.subscriptions === null) {
record.subscriptions = [];
}
record.subscriptions.push(suspenseyCommitSubscription);
} else {
throw new Error(
'Did not expect this host component to be visited when suspending ' +
'the commit. Did you check the SuspendCommit flag?',
);
}
return suspenseyCommitSubscription;
}

function waitForCommitToBeReady():
Expand Down Expand Up @@ -569,38 +570,42 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
callback(endTime);
},

shouldSuspendCommit(type: string, props: Props): boolean {
if (type === 'suspensey-thing' && typeof props.src === 'string') {
if (suspenseyThingCache === null) {
suspenseyThingCache = new Map();
}
const record = suspenseyThingCache.get(props.src);
if (record === undefined) {
const newRecord: SuspenseyThingRecord = {
status: 'pending',
subscriptions: null,
};
suspenseyThingCache.set(props.src, newRecord);
const onLoadStart = props.onLoadStart;
if (typeof onLoadStart === 'function') {
onLoadStart();
}
return props.src;
} else {
if (record.status === 'pending') {
// The resource was already requested, but it hasn't finished
// loading yet.
return true;
} else {
// The resource has already loaded. If the renderer is confident that
// the resource will still be cached by the time the render commits,
// then it can return false, like we do here.
return false;
}
maySuspendCommit(type: string, props: Props): boolean {
// Asks whether it's possible for this combination of type and props
// to ever need to suspend. This is different from asking whether it's
// currently ready because even if it's ready now, it might get purged
// from the cache later.
return type === 'suspensey-thing' && typeof props.src === 'string';
},

preloadInstance(type: string, props: Props): boolean {
if (type !== 'suspensey-thing' || typeof props.src !== 'string') {
throw new Error('Attempted to preload unexpected instance: ' + type);
}

// In addition to preloading an instance, this method asks whether the
// instance is ready to be committed. If it's not, React may yield to the
// main thread and ask again. It's possible a load event will fire in
// between, in which case we can avoid showing a fallback.
if (suspenseyThingCache === null) {
suspenseyThingCache = new Map();
}
const record = suspenseyThingCache.get(props.src);
if (record === undefined) {
const newRecord: SuspenseyThingRecord = {
status: 'pending',
subscriptions: null,
};
suspenseyThingCache.set(props.src, newRecord);
const onLoadStart = props.onLoadStart;
if (typeof onLoadStart === 'function') {
onLoadStart();
}
return false;
} else {
// If this is false, React will trigger a fallback, if needed.
return record.status === 'fulfilled';
}
// Don't need to suspend.
return false;
},

startSuspendingCommit,
Expand Down
63 changes: 44 additions & 19 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ import {
finalizeContainerChildren,
preparePortalMount,
prepareScopeUpdate,
shouldSuspendCommit,
maySuspendCommit,
preloadInstance,
} from './ReactFiberHostConfig';
import {
getRootHostContainer,
Expand Down Expand Up @@ -434,8 +435,6 @@ function updateHostComponent(
// Even better would be if children weren't special cased at all tho.
const instance: Instance = workInProgress.stateNode;

suspendHostCommitIfNeeded(workInProgress, type, newProps, renderLanes);

const currentHostContext = getHostContext();
// TODO: Experiencing an error where oldProps is null. Suggests a host
// component is hitting the resume path. Figure out why. Possibly
Expand Down Expand Up @@ -495,8 +494,6 @@ function updateHostComponent(
recyclableInstance,
);

suspendHostCommitIfNeeded(workInProgress, type, newProps, renderLanes);

if (
finalizeInitialChildren(newInstance, type, newProps, currentHostContext)
) {
Expand All @@ -519,17 +516,17 @@ function updateHostComponent(
// not created until the complete phase. For our existing use cases, host nodes
// that suspend don't have children, so it doesn't matter. But that might not
// always be true in the future.
function suspendHostCommitIfNeeded(
function preloadInstanceAndSuspendIfNeeded(
workInProgress: Fiber,
type: Type,
props: Props,
renderLanes: Lanes,
) {
// Ask the renderer if this instance should suspend the commit.
if (!shouldSuspendCommit(type, props)) {
if (!maySuspendCommit(type, props)) {
// If this flag was set previously, we can remove it. The flag represents
// whether this particular set of props might ever need to suspend. The
// safest thing to do is for shouldSuspendCommit to always return true, but
// safest thing to do is for maySuspendCommit to always return true, but
// if the renderer is reasonably confident that the underlying resource
// won't be evicted, it can return false as a performance optimization.
workInProgress.flags &= ~SuspenseyCommit;
Expand All @@ -552,16 +549,24 @@ function suspendHostCommitIfNeeded(
// TODO: We may decide to expose a way to force a fallback even during a
// sync update.
if (!includesOnlyNonUrgentLanes(renderLanes)) {
// This is an urgent render. Never suspend or trigger a fallback.
// This is an urgent render. Don't suspend or show a fallback. Also,
// there's no need to preload, because we're going to commit this
// synchronously anyway.
// TODO: Could there be benefit to preloading even during a synchronous
// render? The main thread will be blocked until the commit phase, but
// maybe the browser would be able to start loading off thread anyway?
// Likely a micro-optimization either way because typically new content
// is loaded during a transition, not an urgent render.
} else {
// Need to decide whether to activate the nearest fallback or to continue
// rendering and suspend right before the commit phase.
if (shouldRemainOnPreviousScreen()) {
// It's OK to block the commit. Don't show a fallback.
} else {
// We shouldn't block the commit. Activate a fallback at the nearest
// Suspense boundary.
suspendCommit();
// Preload the instance
const isReady = preloadInstance(type, props);
if (!isReady) {
if (shouldRemainOnPreviousScreen()) {
// It's OK to suspend. Continue rendering.
} else {
// Trigger a fallback rather than block the render.
suspendCommit();
}
}
}
}
Expand Down Expand Up @@ -1054,6 +1059,17 @@ function completeWork(
);
}
bubbleProperties(workInProgress);

// This must come at the very end of the complete phase, because it might
// throw to suspend, and if the resource immediately loads, the work loop
// will resume rendering as if the work-in-progress completed. So it must
// fully complete.
preloadInstanceAndSuspendIfNeeded(
workInProgress,
workInProgress.type,
workInProgress.pendingProps,
renderLanes,
);
return null;
}
}
Expand Down Expand Up @@ -1192,14 +1208,23 @@ function completeWork(
}
}

suspendHostCommitIfNeeded(workInProgress, type, newProps, renderLanes);

if (workInProgress.ref !== null) {
// If there is a ref on a host node we need to schedule a callback
markRef(workInProgress);
}
}
bubbleProperties(workInProgress);

// This must come at the very end of the complete phase, because it might
// throw to suspend, and if the resource immediately loads, the work loop
// will resume rendering as if the work-in-progress completed. So it must
// fully complete.
preloadInstanceAndSuspendIfNeeded(
workInProgress,
type,
newProps,
renderLanes,
);
return null;
}
case HostText: {
Expand Down
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactFiberThenable.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ export const SuspenseException: mixed = new Error(
"call the promise's `.catch` method and pass the result to `use`",
);

export const SuspenseyCommitException: mixed = new Error(
'Suspense Exception: This is not a real error, and should not leak into ' +
"userspace. If you're seeing this, it's likely a bug in React.",
);

// This is a noop thenable that we use to trigger a fallback in throwException.
// TODO: It would be better to refactor throwException into multiple functions
// so we can trigger a fallback directly without having to check the type. But
Expand Down Expand Up @@ -151,7 +156,7 @@ export function suspendCommit(): void {
// noopSuspenseyCommitThenable through to throwException.
// TODO: Factor the thenable check out of throwException
suspendedThenable = noopSuspenseyCommitThenable;
throw SuspenseException;
throw SuspenseyCommitException;
}

// This is used to track the actual thenable that suspended so it can be
Expand Down
Loading