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

Unwrap sync resolved thenables without suspending #25615

Merged
merged 2 commits into from
Nov 2, 2022
Merged

Unwrap sync resolved thenables without suspending #25615

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
35deb4d
Unwrap sync resolved thenables without suspending
acdlite Nov 2, 2022
9e09e09
Clean up code that's no longer needed
acdlite Nov 2, 2022
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
27 changes: 27 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5300,6 +5300,33 @@ describe('ReactDOMFizzServer', () => {
expect(Scheduler).toFlushAndYield([]);
expect(getVisibleChildren(container)).toEqual('Hi');
});

// @gate enableUseHook
it('unwraps thenable that fulfills synchronously without suspending', async () => {
function App() {
const thenable = {
then(resolve) {
// This thenable immediately resolves, synchronously, without waiting
// a microtask.
resolve('Hi');
},
};
try {
return <Text text={use(thenable)} />;
} catch {
throw new Error(
'`use` should not suspend because the thenable resolved synchronously.',
);
}
}
// Because the thenable resolves synchronously, we should be able to finish
// rendering synchronously, with no fallback.
await act(async () => {
const {pipe} = renderToPipeableStream(<App />);
pipe(writable);
});
expect(getVisibleChildren(container)).toEqual('Hi');
});
});

describe('useEvent', () => {
Expand Down
18 changes: 15 additions & 3 deletions packages/react-reconciler/src/ReactFiberThenable.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,24 +110,36 @@ export function trackUsedThenable<T>(thenable: Thenable<T>, index: number): T {
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
} else {
const pendingThenable: PendingThenable<mixed> = (thenable: any);
const pendingThenable: PendingThenable<T> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
fulfilledValue => {
if (thenable.status === 'pending') {
const fulfilledThenable: FulfilledThenable<mixed> = (thenable: any);
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
fulfilledThenable.status = 'fulfilled';
fulfilledThenable.value = fulfilledValue;
}
},
(error: mixed) => {
if (thenable.status === 'pending') {
const rejectedThenable: RejectedThenable<mixed> = (thenable: any);
const rejectedThenable: RejectedThenable<T> = (thenable: any);
rejectedThenable.status = 'rejected';
rejectedThenable.reason = error;
}
},
);

// Check one more time in case the thenable resolved synchronously
switch (thenable.status) {
Copy link

@XiNiHa XiNiHa Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@acdlite this never gets hit, since this always results in 'pending':

const syncThen = (p) => {
  let status = 'pending'
  p.then(() => status = 'fulfilled')
  console.log(status)
}
syncThen(Promise.resolve()) // 'pending'
syncThen(new Promise(r => r())) // 'pending'
const resolved = new Promise(r => r())
syncThen(resolved) // 'pending'
setTimeout(() => syncThen(resolved), 0) // 'pending'

I believe it's because .then always gets executed asynchronously, like setTimeout(..., 0)

(I found this while testing with use(Promise.resolve()) in the experimental version)

case 'fulfilled': {
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
return fulfilledThenable.value;
}
case 'rejected': {
const rejectedThenable: RejectedThenable<T> = (thenable: any);
throw rejectedThenable.reason;
}
}
}

// Suspend.
Expand Down
18 changes: 15 additions & 3 deletions packages/react-reconciler/src/ReactFiberThenable.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,24 +110,36 @@ export function trackUsedThenable<T>(thenable: Thenable<T>, index: number): T {
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
} else {
const pendingThenable: PendingThenable<mixed> = (thenable: any);
const pendingThenable: PendingThenable<T> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
fulfilledValue => {
if (thenable.status === 'pending') {
const fulfilledThenable: FulfilledThenable<mixed> = (thenable: any);
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
fulfilledThenable.status = 'fulfilled';
fulfilledThenable.value = fulfilledValue;
}
},
(error: mixed) => {
if (thenable.status === 'pending') {
const rejectedThenable: RejectedThenable<mixed> = (thenable: any);
const rejectedThenable: RejectedThenable<T> = (thenable: any);
rejectedThenable.status = 'rejected';
rejectedThenable.reason = error;
}
},
);

// Check one more time in case the thenable resolved synchronously
switch (thenable.status) {
case 'fulfilled': {
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
return fulfilledThenable.value;
}
case 'rejected': {
const rejectedThenable: RejectedThenable<T> = (thenable: any);
throw rejectedThenable.reason;
}
}
}

// Suspend.
Expand Down
36 changes: 10 additions & 26 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 @@ -2016,37 +2016,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) {
workInProgressSuspendedReason !== NotSuspended &&
workInProgress !== null
) {
// The work loop is suspended. We need to either unwind the stack or
// replay the suspended component.
// The work loop is suspended. During a synchronous render, we don't
// yield to the main thread. Immediately unwind the stack. This will
// trigger either a fallback or an error boundary.
// TODO: For discrete and "default" updates (anything that's not
// flushSync), we want to wait for the microtasks the flush before
// unwinding. Will probably implement this using renderRootConcurrent,
// or merge renderRootSync and renderRootConcurrent into the same
// function and fork the behavior some other way.
const unitOfWork = workInProgress;
const thrownValue = workInProgressThrownValue;
workInProgressSuspendedReason = NotSuspended;
workInProgressThrownValue = null;
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);

// TODO: This check is only here to account for thenables that
// synchronously resolve. Otherwise we would always unwind when
// rendering with renderRootSync. (In the future, discrete updates will
// use renderRootConcurrent instead.) We should account for
// synchronously resolved thenables before hitting this path.
switch (workInProgressSuspendedReason) {
case SuspendedOnError: {
// Unwind then continue with the normal work loop.
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
break;
}
default: {
const wasPinged =
workInProgressSuspendedThenableState !== null &&
isThenableStateResolved(workInProgressSuspendedThenableState);
if (wasPinged) {
replaySuspendedUnitOfWork(unitOfWork, thrownValue);
} else {
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
}
// Continue with the normal work loop.
break;
}
}
// Continue with the normal work loop.
}
workLoopSync();
break;
Expand Down
36 changes: 10 additions & 26 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2016,37 +2016,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) {
workInProgressSuspendedReason !== NotSuspended &&
workInProgress !== null
) {
// The work loop is suspended. We need to either unwind the stack or
// replay the suspended component.
// The work loop is suspended. During a synchronous render, we don't
// yield to the main thread. Immediately unwind the stack. This will
// trigger either a fallback or an error boundary.
// TODO: For discrete and "default" updates (anything that's not
// flushSync), we want to wait for the microtasks the flush before
// unwinding. Will probably implement this using renderRootConcurrent,
// or merge renderRootSync and renderRootConcurrent into the same
// function and fork the behavior some other way.
const unitOfWork = workInProgress;
const thrownValue = workInProgressThrownValue;
workInProgressSuspendedReason = NotSuspended;
workInProgressThrownValue = null;
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);

// TODO: This check is only here to account for thenables that
// synchronously resolve. Otherwise we would always unwind when
// rendering with renderRootSync. (In the future, discrete updates will
// use renderRootConcurrent instead.) We should account for
// synchronously resolved thenables before hitting this path.
switch (workInProgressSuspendedReason) {
case SuspendedOnError: {
// Unwind then continue with the normal work loop.
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
break;
}
default: {
const wasPinged =
workInProgressSuspendedThenableState !== null &&
isThenableStateResolved(workInProgressSuspendedThenableState);
if (wasPinged) {
replaySuspendedUnitOfWork(unitOfWork, thrownValue);
} else {
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
}
// Continue with the normal work loop.
break;
}
}
// Continue with the normal work loop.
}
workLoopSync();
break;
Expand Down
28 changes: 28 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactThenable-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,4 +640,32 @@ describe('ReactThenable', () => {
});
expect(Scheduler).toHaveYielded(['Something different']);
});

// @gate enableUseHook
test('unwraps thenable that fulfills synchronously without suspending', async () => {
function App() {
const thenable = {
then(resolve) {
// This thenable immediately resolves, synchronously, without waiting
// a microtask.
resolve('Hi');
},
};
try {
return <Text text={use(thenable)} />;
} catch {
throw new Error(
'`use` should not suspend because the thenable resolved synchronously.',
);
}
}
// Because the thenable resolves synchronously, we should be able to finish
// rendering synchronously, with no fallback.
const root = ReactNoop.createRoot();
ReactNoop.flushSync(() => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['Hi']);
expect(root).toMatchRenderedOutput('Hi');
});
});
36 changes: 36 additions & 0 deletions packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,4 +778,40 @@ describe('ReactFlightDOMBrowser', () => {
});
expect(container.innerHTML).toBe('Hi');
});

// @gate enableUseHook
it('unwraps thenable that fulfills synchronously without suspending', async () => {
function Server() {
const thenable = {
then(resolve) {
// This thenable immediately resolves, synchronously, without waiting
// a microtask.
resolve('Hi');
},
};
try {
return use(thenable);
} catch {
throw new Error(
'`use` should not suspend because the thenable resolved synchronously.',
);
}
}

// Because the thenable resolves synchronously, we should be able to finish
// rendering synchronously, with no fallback.
const stream = ReactServerDOMWriter.renderToReadableStream(<Server />);
const response = ReactServerDOMReader.createFromReadableStream(stream);

function Client() {
return use(response);
}

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await act(async () => {
root.render(<Client />);
});
expect(container.innerHTML).toBe('Hi');
});
});
18 changes: 15 additions & 3 deletions packages/react-server/src/ReactFizzThenable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,24 +83,36 @@ export function trackUsedThenable<T>(
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
} else {
const pendingThenable: PendingThenable<mixed> = (thenable: any);
const pendingThenable: PendingThenable<T> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
fulfilledValue => {
if (thenable.status === 'pending') {
const fulfilledThenable: FulfilledThenable<mixed> = (thenable: any);
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
fulfilledThenable.status = 'fulfilled';
fulfilledThenable.value = fulfilledValue;
}
},
(error: mixed) => {
if (thenable.status === 'pending') {
const rejectedThenable: RejectedThenable<mixed> = (thenable: any);
const rejectedThenable: RejectedThenable<T> = (thenable: any);
rejectedThenable.status = 'rejected';
rejectedThenable.reason = error;
}
},
);

// Check one more time in case the thenable resolved synchronously
switch (thenable.status) {
case 'fulfilled': {
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
return fulfilledThenable.value;
}
case 'rejected': {
const rejectedThenable: RejectedThenable<T> = (thenable: any);
throw rejectedThenable.reason;
}
}
}

// Suspend.
Expand Down
18 changes: 15 additions & 3 deletions packages/react-server/src/ReactFlightThenable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,24 +83,36 @@ export function trackUsedThenable<T>(
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
} else {
const pendingThenable: PendingThenable<mixed> = (thenable: any);
const pendingThenable: PendingThenable<T> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
fulfilledValue => {
if (thenable.status === 'pending') {
const fulfilledThenable: FulfilledThenable<mixed> = (thenable: any);
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
fulfilledThenable.status = 'fulfilled';
fulfilledThenable.value = fulfilledValue;
}
},
(error: mixed) => {
if (thenable.status === 'pending') {
const rejectedThenable: RejectedThenable<mixed> = (thenable: any);
const rejectedThenable: RejectedThenable<T> = (thenable: any);
rejectedThenable.status = 'rejected';
rejectedThenable.reason = error;
}
},
);

// Check one more time in case the thenable resolved synchronously
switch (thenable.status) {
case 'fulfilled': {
const fulfilledThenable: FulfilledThenable<T> = (thenable: any);
return fulfilledThenable.value;
}
case 'rejected': {
const rejectedThenable: RejectedThenable<T> = (thenable: any);
throw rejectedThenable.reason;
}
}
}

// Suspend.
Expand Down