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

Simplify Webpack References by encoding file path + export name as single id #26300

Merged
merged 2 commits into from
Mar 5, 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
Binary file added fixtures/flight/public/favicon.ico
Binary file not shown.
4 changes: 2 additions & 2 deletions fixtures/flight/server/region.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ app.post('/', bodyParser.text(), async function (req, res) {
const {renderToPipeableStream} = await import(
'react-server-dom-webpack/server'
);
const serverReference = JSON.parse(req.get('rsc-action'));
const {filepath, name} = serverReference;
const serverReference = req.get('rsc-action');
const [filepath, name] = serverReference.split('#');
const action = (await import(filepath))[name];
// Validate that this is actually a function we intended to expose and
// not the client trying to invoke arbitrary functions. In a real app,
Expand Down
2 changes: 1 addition & 1 deletion fixtures/flight/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ let data = ReactServerDOMReader.createFromFetch(
method: 'POST',
headers: {
Accept: 'text/x-component',
'rsc-action': JSON.stringify({filepath: id.id, name: id.name}),
'rsc-action': id,
},
body: JSON.stringify(args),
});
Expand Down
10 changes: 6 additions & 4 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ function createModelReject<T>(chunk: SomeChunk<T>): (error: mixed) => void {

function createServerReferenceProxy<A: Iterable<any>, T>(
response: Response,
metaData: any,
metaData: {id: any, bound: Thenable<Array<any>>},
): (...A) => Promise<T> {
const callServer = response._callServer;
const proxy = function (): Promise<T> {
Expand All @@ -482,12 +482,14 @@ function createServerReferenceProxy<A: Iterable<any>, T>(
const p = metaData.bound;
if (p.status === INITIALIZED) {
const bound = p.value;
return callServer(metaData, bound.concat(args));
return callServer(metaData.id, bound.concat(args));
}
// Since this is a fake Promise whose .then doesn't chain, we have to wrap it.
// TODO: Remove the wrapper once that's fixed.
return Promise.resolve(p).then(function (bound) {
return callServer(metaData, bound.concat(args));
return ((Promise.resolve(p): any): Promise<Array<any>>).then(function (
bound,
) {
return callServer(metaData.id, bound.concat(args));
});
};
return proxy;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import isArray from 'shared/isArray';

export type ClientReference<T> = JSResourceReference<T>;
export type ServerReference<T> = T;
export type ServerReferenceMetadata = {};
export type ServerReferenceId = {};

import type {
Destination,
Expand Down Expand Up @@ -69,7 +69,7 @@ export function resolveClientReferenceMetadata<T>(
export function resolveServerReferenceMetadata<T>(
config: BundlerConfig,
resource: ServerReference<T>,
): ServerReferenceMetadata {
): {id: ServerReferenceId, bound: Promise<Array<any>>} {
throw new Error('Not implemented.');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ export opaque type ClientReferenceMetadata = {
id: string,
chunks: Array<string>,
name: string,
async: boolean,
};

// eslint-disable-next-line no-unused-vars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,7 @@ import {
close,
} from 'react-client/src/ReactFlightClientStream';

type CallServerCallback = <A, T>(
{filepath: string, name: string},
args: A,
) => Promise<T>;
type CallServerCallback = <A, T>(string, args: A) => Promise<T>;

export type Options = {
callServer?: CallServerCallback,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,24 @@
import type {ReactModel} from 'react-server/src/ReactFlightServer';

type WebpackMap = {
[filepath: string]: {
[name: string]: ClientReferenceMetadata,
},
[id: string]: ClientReferenceMetadata,
};

export type BundlerConfig = WebpackMap;

export type ServerReference<T: Function> = T & {
$$typeof: symbol,
$$filepath: string,
$$name: string,
$$id: string,
$$bound: Array<ReactModel>,
};

export type ServerReferenceMetadata = {
id: string,
name: string,
bound: Promise<Array<ReactModel>>,
};
export type ServerReferenceId = string;

// eslint-disable-next-line no-unused-vars
export type ClientReference<T> = {
$$typeof: symbol,
filepath: string,
name: string,
async: boolean,
$$id: string,
$$async: boolean,
};

export type ClientReferenceMetadata = {
Expand All @@ -53,12 +45,7 @@ const SERVER_REFERENCE_TAG = Symbol.for('react.server.reference');
export function getClientReferenceKey(
reference: ClientReference<any>,
): ClientReferenceKey {
return (
reference.filepath +
'#' +
reference.name +
(reference.async ? '#async' : '')
);
return reference.$$async ? reference.$$id + '#async' : reference.$$id;
}

export function isClientReference(reference: Object): boolean {
Expand All @@ -73,9 +60,8 @@ export function resolveClientReferenceMetadata<T>(
config: BundlerConfig,
clientReference: ClientReference<T>,
): ClientReferenceMetadata {
const resolvedModuleData =
config[clientReference.filepath][clientReference.name];
if (clientReference.async) {
const resolvedModuleData = config[clientReference.$$id];
if (clientReference.$$async) {
return {
id: resolvedModuleData.id,
chunks: resolvedModuleData.chunks,
Expand All @@ -90,10 +76,9 @@ export function resolveClientReferenceMetadata<T>(
export function resolveServerReferenceMetadata<T>(
config: BundlerConfig,
serverReference: ServerReference<T>,
): ServerReferenceMetadata {
): {id: ServerReferenceId, bound: Promise<Array<any>>} {
return {
id: serverReference.$$filepath,
name: serverReference.$$name,
id: serverReference.$$id,
bound: Promise.resolve(serverReference.$$bound),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,7 @@ function transformServerModule(
}
newSrc += 'Object.defineProperties(' + local + ',{';
newSrc += '$$typeof: {value: Symbol.for("react.server.reference")},';
newSrc += '$$filepath: {value: ' + JSON.stringify(url) + '},';
newSrc += '$$name: { value: ' + JSON.stringify(exported) + '},';
newSrc += '$$id: {value: ' + JSON.stringify(url + '#' + exported) + '},';
newSrc += '$$bound: { value: [] }';
newSrc += '});\n';
});
Expand Down Expand Up @@ -343,9 +342,8 @@ async function transformClientModule(
');';
}
newSrc += '},{';
newSrc += 'name: { value: ' + JSON.stringify(name) + '},';
newSrc += '$$typeof: {value: CLIENT_REFERENCE},';
newSrc += 'filepath: {value: ' + JSON.stringify(url) + '}';
newSrc += '$$id: {value: ' + JSON.stringify(url + '#' + name) + '}';
newSrc += '});\n';
}
return newSrc;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ module.exports = function register() {
// $FlowFixMe[method-unbinding]
const args = Array.prototype.slice.call(arguments, 1);
newFn.$$typeof = SERVER_REFERENCE;
newFn.$$filepath = this.$$filepath;
newFn.$$name = this.$$name;
newFn.$$id = this.$$id;
newFn.$$bound = this.$$bound.concat(args);
}
return newFn;
Expand All @@ -44,14 +43,14 @@ module.exports = function register() {
// These names are a little too common. We should probably have a way to
// have the Flight runtime extract the inner target instead.
return target.$$typeof;
case 'filepath':
return target.filepath;
case '$$id':
return target.$$id;
case '$$async':
return target.$$async;
case 'name':
return target.name;
case 'displayName':
return undefined;
case 'async':
return target.async;
// We need to special case this because createElement reads it if we pass this
// reference.
case 'defaultProps':
Expand All @@ -69,20 +68,8 @@ module.exports = function register() {
`that itself renders a Client Context Provider.`,
);
}
let expression;
switch (target.name) {
case '':
// eslint-disable-next-line react-internal/safe-string-coercion
expression = String(name);
break;
case '*':
// eslint-disable-next-line react-internal/safe-string-coercion
expression = String(name);
break;
default:
// eslint-disable-next-line react-internal/safe-string-coercion
expression = String(target.name) + '.' + String(name);
}
// eslint-disable-next-line react-internal/safe-string-coercion
const expression = String(target.name) + '.' + String(name);
throw new Error(
`Cannot access ${expression} on the server. ` +
'You cannot dot into a client module from a server component. ' +
Expand All @@ -103,15 +90,13 @@ module.exports = function register() {
switch (name) {
// These names are read by the Flight runtime if you end up using the exports object.
case '$$typeof':
// These names are a little too common. We should probably have a way to
// have the Flight runtime extract the inner target instead.
return target.$$typeof;
case 'filepath':
return target.filepath;
case '$$id':
return target.$$id;
case '$$async':
return target.$$async;
case 'name':
return target.name;
case 'async':
return target.async;
// We need to special case this because createElement reads it if we pass this
// reference.
case 'defaultProps':
Expand All @@ -125,7 +110,7 @@ module.exports = function register() {
case '__esModule':
// Something is conditionally checking which export to use. We'll pretend to be
// an ESM compat module but then we'll check again on the client.
const moduleId = target.filepath;
const moduleId = target.$$id;
target.default = Object.defineProperties(
(function () {
throw new Error(
Expand All @@ -136,12 +121,11 @@ module.exports = function register() {
);
}: any),
{
$$typeof: {value: CLIENT_REFERENCE},
// This a placeholder value that tells the client to conditionally use the
// whole object or just the default export.
name: {value: ''},
$$typeof: {value: CLIENT_REFERENCE},
filepath: {value: target.filepath},
async: {value: target.async},
$$id: {value: target.$$id + '#'},
$$async: {value: target.$$async},
},
);
return true;
Expand All @@ -150,17 +134,15 @@ module.exports = function register() {
// Use a cached value
return target.then;
}
if (!target.async) {
if (!target.$$async) {
// If this module is expected to return a Promise (such as an AsyncModule) then
// we should resolve that with a client reference that unwraps the Promise on
// the client.

const clientReference = Object.defineProperties(({}: any), {
// Represents the whole Module object instead of a particular import.
name: {value: '*'},
$$typeof: {value: CLIENT_REFERENCE},
filepath: {value: target.filepath},
async: {value: true},
$$id: {value: target.$$id},
$$async: {value: true},
});
const proxy = new Proxy(clientReference, proxyHandlers);

Expand All @@ -176,10 +158,9 @@ module.exports = function register() {
// If this is not used as a Promise but is treated as a reference to a `.then`
// export then we should treat it as a reference to that name.
{
name: {value: 'then'},
$$typeof: {value: CLIENT_REFERENCE},
filepath: {value: target.filepath},
async: {value: false},
$$id: {value: target.$$id + '#then'},
$$async: {value: false},
},
));
return then;
Expand All @@ -206,8 +187,8 @@ module.exports = function register() {
{
name: {value: name},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I had the same thought and created #26322.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I left it intentionally for debugging purposes since these are functions and they're printed with their name property. The others either have intentionally empty names or hard coded names like "then".

$$typeof: {value: CLIENT_REFERENCE},
filepath: {value: target.filepath},
async: {value: target.async},
$$id: {value: target.$$id + '#' + name},
$$async: {value: target.$$async},
},
);
cachedReference = target[name] = new Proxy(
Expand Down Expand Up @@ -284,11 +265,10 @@ module.exports = function register() {
if (useClient) {
const moduleId: string = (url.pathToFileURL(filename).href: any);
const clientReference = Object.defineProperties(({}: any), {
// Represents the whole Module object instead of a particular import.
name: {value: '*'},
$$typeof: {value: CLIENT_REFERENCE},
filepath: {value: moduleId},
async: {value: false},
// Represents the whole Module object instead of a particular import.
$$id: {value: moduleId},
$$async: {value: false},
});
// $FlowFixMe[incompatible-call] found when upgrading Flow
this.exports = new Proxy(clientReference, proxyHandlers);
Expand All @@ -306,10 +286,9 @@ module.exports = function register() {
if (typeof exports === 'function') {
// The module exports a function directly,
Object.defineProperties((exports: any), {
// Represents the whole Module object instead of a particular import.
$$typeof: {value: SERVER_REFERENCE},
$$filepath: {value: moduleId},
$$name: {value: '*'},
// Represents the whole Module object instead of a particular import.
$$id: {value: moduleId},
$$bound: {value: []},
});
} else {
Expand All @@ -320,8 +299,7 @@ module.exports = function register() {
if (typeof value === 'function') {
Object.defineProperties((value: any), {
$$typeof: {value: SERVER_REFERENCE},
$$filepath: {value: moduleId},
$$name: {value: key},
$$id: {value: moduleId + '#' + key},
$$bound: {value: []},
});
}
Expand Down
Loading