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

[Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises #25260

Merged
merged 8 commits into from
Sep 15, 2022
Merged

[Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises #25260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
73e6a44
[Flight] Align Chunks with Thenable used with experimental_use
sebmarkbage Aug 27, 2022
23920ea
Implement Thenable Protocol on Chunks
sebmarkbage Aug 27, 2022
744b7bb
Subclass Promises
sebmarkbage Sep 12, 2022
fda26be
Pass key through JSON parsing
sebmarkbage Aug 28, 2022
372c008
Wait for preloadModules before resolving module chunks
sebmarkbage Sep 9, 2022
737b236
Initialize lazy resolved values before reading the result
sebmarkbage Aug 29, 2022
b41cf4f
Block a model from initializing if its direct dependencies are pending
sebmarkbage Sep 14, 2022
1c481e9
Treat blocked modules or models as a special status
sebmarkbage Sep 14, 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
407 changes: 309 additions & 98 deletions packages/react-client/src/ReactFlightClient.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/react-client/src/ReactFlightClientStream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ function createFromJSONCallback(response: Response) {
return function(key: string, value: JSONValue) {
if (typeof value === 'string') {
// We can't use .bind here because we need the "this" value.
return parseModelString(response, this, value);
return parseModelString(response, this, key, value);
}
if (typeof value === 'object' && value !== null) {
return parseModelTuple(response, value);
Expand Down
13 changes: 6 additions & 7 deletions packages/react-reconciler/src/ReactFiberWakeable.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -75,9 +71,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
suspendedThenable = null;
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down
13 changes: 6 additions & 7 deletions packages/react-reconciler/src/ReactFiberWakeable.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -75,9 +71,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
suspendedThenable = null;
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
sebmarkbage marked this conversation as resolved.
Show resolved Hide resolved
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down
8 changes: 5 additions & 3 deletions packages/react-server-dom-relay/src/ReactFlightDOMRelayClientHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ export function resolveModuleReference<T>(
return resolveModuleReferenceImpl(moduleData);
}

function parseModelRecursively(response: Response, parentObj, value) {
function parseModelRecursively(response: Response, parentObj, key, value) {
if (typeof value === 'string') {
return parseModelString(response, parentObj, value);
return parseModelString(response, parentObj, key, value);
}
if (typeof value === 'object' && value !== null) {
if (isArray(value)) {
Expand All @@ -55,6 +55,7 @@ function parseModelRecursively(response: Response, parentObj, value) {
(parsedValue: any)[i] = parseModelRecursively(
response,
value,
'' + i,
value[i],
);
}
Expand All @@ -65,6 +66,7 @@ function parseModelRecursively(response: Response, parentObj, value) {
(parsedValue: any)[innerKey] = parseModelRecursively(
response,
value,
innerKey,
value[innerKey],
);
}
Expand All @@ -77,5 +79,5 @@ function parseModelRecursively(response: Response, parentObj, value) {
const dummy = {};

export function parseModel<T>(response: Response, json: UninitializedModel): T {
return (parseModelRecursively(response, dummy, json): any);
return (parseModelRecursively(response, dummy, '', json): any);
}
68 changes: 38 additions & 30 deletions packages/react-server-dom-webpack/src/ReactFlightClientWebpackBundlerConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
* @flow
*/

import type {Thenable} from 'shared/ReactTypes';
import type {
Thenable,
FulfilledThenable,
RejectedThenable,
} from 'shared/ReactTypes';

export type WebpackSSRMap = {
[clientId: string]: {
Expand Down Expand Up @@ -56,7 +60,9 @@ const asyncModuleCache: Map<string, Thenable<any>> = new Map();

// Start preloading the modules since we might need them soon.
// This function doesn't suspend.
export function preloadModule<T>(moduleData: ModuleReference<T>): void {
export function preloadModule<T>(
moduleData: ModuleReference<T>,
): null | Thenable<any> {
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This changes the protocol of preloading modules. It has to suspend here and not in require module. This is not updated in ReactFlightDOMRelayClientIntegration because they live in www. So someone else has to update it.

https://github.com/facebook/react/blob/main/packages/react-server-dom-relay/src/__mocks__/ReactFlightDOMRelayClientIntegration.js#L16-L19

I don't think it's currently used there so it's not blocking.

The protocol is that requireModule can no longer suspend by throwing a Promise. Instead, preloadModule should return a Promise that resolves when it's safe to call requireModule synchronously.

@frandiox this will also affect the Vite parts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @kassens

const chunks = moduleData.chunks;
const promises = [];
for (let i = 0; i < chunks.length; i++) {
Expand All @@ -72,20 +78,35 @@ export function preloadModule<T>(moduleData: ModuleReference<T>): void {
}
}
if (moduleData.async) {
const modulePromise: any = Promise.all(promises).then(() => {
return __webpack_require__(moduleData.id);
});
modulePromise.then(
value => {
modulePromise.status = 'fulfilled';
modulePromise.value = value;
},
reason => {
modulePromise.status = 'rejected';
modulePromise.reason = reason;
},
);
asyncModuleCache.set(moduleData.id, modulePromise);
const existingPromise = asyncModuleCache.get(moduleData.id);
if (existingPromise) {
if (existingPromise.status === 'fulfilled') {
return null;
}
return existingPromise;
} else {
const modulePromise: Thenable<T> = Promise.all(promises).then(() => {
return __webpack_require__(moduleData.id);
});
modulePromise.then(
value => {
const fulfilledThenable: FulfilledThenable<mixed> = (modulePromise: any);
fulfilledThenable.status = 'fulfilled';
fulfilledThenable.value = value;
},
reason => {
const rejectedThenable: RejectedThenable<mixed> = (modulePromise: any);
rejectedThenable.status = 'rejected';
rejectedThenable.reason = reason;
},
);
asyncModuleCache.set(moduleData.id, modulePromise);
return modulePromise;
}
} else if (promises.length > 0) {
return Promise.all(promises);
} else {
return null;
}
}

Expand All @@ -99,23 +120,10 @@ export function requireModule<T>(moduleData: ModuleReference<T>): T {
const promise: any = asyncModuleCache.get(moduleData.id);
if (promise.status === 'fulfilled') {
moduleExports = promise.value;
} else if (promise.status === 'rejected') {
throw promise.reason;
} else {
throw promise;
throw promise.reason;
}
} else {
const chunks = moduleData.chunks;
for (let i = 0; i < chunks.length; i++) {
const chunkId = chunks[i];
const entry = chunkCache.get(chunkId);
if (entry !== null) {
// We assume that preloadModule has been called before.
// So we don't expect to see entry being undefined here, that's an error.
// Let's throw either an error or the Promise.
throw entry;
}
}
moduleExports = __webpack_require__(moduleData.id);
}
if (moduleData.name === '*') {
Expand Down
8 changes: 5 additions & 3 deletions packages/react-server-native-relay/src/ReactFlightNativeRelayClientHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ export function resolveModuleReference<T>(
return resolveModuleReferenceImpl(moduleData);
}

function parseModelRecursively(response: Response, parentObj, value) {
function parseModelRecursively(response: Response, parentObj, key, value) {
if (typeof value === 'string') {
return parseModelString(response, parentObj, value);
return parseModelString(response, parentObj, key, value);
}
if (typeof value === 'object' && value !== null) {
if (isArray(value)) {
Expand All @@ -55,6 +55,7 @@ function parseModelRecursively(response: Response, parentObj, value) {
(parsedValue: any)[i] = parseModelRecursively(
response,
value,
'' + i,
value[i],
);
}
Expand All @@ -65,6 +66,7 @@ function parseModelRecursively(response: Response, parentObj, value) {
(parsedValue: any)[innerKey] = parseModelRecursively(
response,
value,
innerKey,
value[innerKey],
);
}
Expand All @@ -77,5 +79,5 @@ function parseModelRecursively(response: Response, parentObj, value) {
const dummy = {};

export function parseModel<T>(response: Response, json: UninitializedModel): T {
return (parseModelRecursively(response, dummy, json): any);
return (parseModelRecursively(response, dummy, '', json): any);
}
13 changes: 6 additions & 7 deletions packages/react-server/src/ReactFizzWakeable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -57,9 +53,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// TODO: Log a warning?
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down
13 changes: 6 additions & 7 deletions packages/react-server/src/ReactFlightWakeable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -57,9 +53,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// TODO: Log a warning?
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down
4 changes: 2 additions & 2 deletions scripts/flow/react-relay-hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ declare module 'ReactFlightDOMRelayClientIntegration' {
): JSResourceReference<T>;
declare export function preloadModule<T>(
moduleReference: JSResourceReference<T>,
): void;
): null | Promise<void>;
declare export function requireModule<T>(
moduleReference: JSResourceReference<T>,
): T;
Expand Down Expand Up @@ -95,7 +95,7 @@ declare module 'ReactFlightNativeRelayClientIntegration' {
): JSResourceReference<T>;
declare export function preloadModule<T>(
moduleReference: JSResourceReference<T>,
): void;
): null | Promise<void>;
declare export function requireModule<T>(
moduleReference: JSResourceReference<T>,
): T;
Expand Down