From 95f4929a85d036b12e39e599bb3a86ff372ae620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 19 Feb 2024 11:49:25 -0500 Subject: [PATCH] Encode better error messages when part of the context is a client reference (#28355) Alternative to #28354. If a client reference is one of the props being describes as part of another error, we call toString on it, which errors. We should error explicitly when a Symbol prop is extracted. However, pragmatically I added the toString symbol tag even though we don't know what the real tostring will be but we also lie about the typeof. We can however in addition to this give it a different description because describing this property as an object isn't quite right. We probably could extract the export name but that's kind of renderer specific and I just added this shared module to Fizz which doesn't have that which is unfortunate an consequence. For default exports we don't have a good name of what the alias was in the receiver. Could maybe call it "default" but for now I just call it "client". --- .../src/ReactFlightTurbopackReferences.js | 222 ++++++++++-------- .../src/ReactFlightWebpackReferences.js | 24 +- .../src/__tests__/ReactFlightDOM-test.js | 67 ++++++ packages/shared/ReactSerializationErrors.js | 14 ++ 4 files changed, 230 insertions(+), 97 deletions(-) diff --git a/packages/react-server-dom-turbopack/src/ReactFlightTurbopackReferences.js b/packages/react-server-dom-turbopack/src/ReactFlightTurbopackReferences.js index 89985348b2c78..f68c6f27f9640 100644 --- a/packages/react-server-dom-turbopack/src/ReactFlightTurbopackReferences.js +++ b/packages/react-server-dom-turbopack/src/ReactFlightTurbopackReferences.js @@ -92,7 +92,11 @@ export function registerServerReference( const PROMISE_PROTOTYPE = Promise.prototype; const deepProxyHandlers = { - get: function (target: Function, name: string, receiver: Proxy) { + get: function ( + target: Function, + name: string | symbol, + receiver: Proxy, + ) { switch (name) { // These names are read by the Flight runtime if you end up using the exports object. case '$$typeof': @@ -117,6 +121,9 @@ const deepProxyHandlers = { case Symbol.toPrimitive: // $FlowFixMe[prop-missing] return Object.prototype[Symbol.toPrimitive]; + case Symbol.toStringTag: + // $FlowFixMe[prop-missing] + return Object.prototype[Symbol.toStringTag]; case 'Provider': throw new Error( `Cannot render a Client Context Provider on the Server. ` + @@ -137,105 +144,134 @@ const deepProxyHandlers = { }, }; -const proxyHandlers = { - get: function ( - target: Function, - name: string, - receiver: Proxy, - ): $FlowFixMe { - switch (name) { - // These names are read by the Flight runtime if you end up using the exports object. - case '$$typeof': - return target.$$typeof; - case '$$id': - return target.$$id; - case '$$async': - return target.$$async; - case 'name': - return target.name; - // We need to special case this because createElement reads it if we pass this - // reference. - case 'defaultProps': - return undefined; - // Avoid this attempting to be serialized. - case 'toJSON': - return undefined; - case Symbol.toPrimitive: - // $FlowFixMe[prop-missing] - return Object.prototype[Symbol.toPrimitive]; - 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.$$id; - target.default = registerClientReferenceImpl( - (function () { - throw new Error( - `Attempted to call the default export of ${moduleId} from the server ` + - `but it's on the client. It's not possible to invoke a client function from ` + - `the server, it can only be rendered as a Component or passed to props of a ` + - `Client Component.`, - ); - }: any), - target.$$id + '#', - target.$$async, - ); - return true; - case 'then': - if (target.then) { - // Use a cached value - return target.then; - } - 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: ClientReference = - registerClientReferenceImpl(({}: any), target.$$id, true); - const proxy = new Proxy(clientReference, proxyHandlers); - - // Treat this as a resolved Promise for React's use() - target.status = 'fulfilled'; - target.value = proxy; - - const then = (target.then = registerClientReferenceImpl( - (function then(resolve, reject: any) { - // Expose to React. - return Promise.resolve(resolve(proxy)); - }: any), - // 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. - target.$$id + '#then', - false, - )); - return then; - } else { - // Since typeof .then === 'function' is a feature test we'd continue recursing - // indefinitely if we return a function. Instead, we return an object reference - // if we check further. - return undefined; - } - } - let cachedReference = target[name]; - if (!cachedReference) { - const reference: ClientReference = registerClientReferenceImpl( +function getReference(target: Function, name: string | symbol): $FlowFixMe { + switch (name) { + // These names are read by the Flight runtime if you end up using the exports object. + case '$$typeof': + return target.$$typeof; + case '$$id': + return target.$$id; + case '$$async': + return target.$$async; + case 'name': + return target.name; + // We need to special case this because createElement reads it if we pass this + // reference. + case 'defaultProps': + return undefined; + // Avoid this attempting to be serialized. + case 'toJSON': + return undefined; + case Symbol.toPrimitive: + // $FlowFixMe[prop-missing] + return Object.prototype[Symbol.toPrimitive]; + case Symbol.toStringTag: + // $FlowFixMe[prop-missing] + return Object.prototype[Symbol.toStringTag]; + 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.$$id; + target.default = registerClientReferenceImpl( (function () { throw new Error( - // eslint-disable-next-line react-internal/safe-string-coercion - `Attempted to call ${String(name)}() from the server but ${String( - name, - )} is on the client. ` + - `It's not possible to invoke a client function from the server, it can ` + - `only be rendered as a Component or passed to props of a Client Component.`, + `Attempted to call the default export of ${moduleId} from the server ` + + `but it's on the client. It's not possible to invoke a client function from ` + + `the server, it can only be rendered as a Component or passed to props of a ` + + `Client Component.`, ); }: any), - target.$$id + '#' + name, + target.$$id + '#', target.$$async, ); - Object.defineProperty((reference: any), 'name', {value: name}); - cachedReference = target[name] = new Proxy(reference, deepProxyHandlers); + return true; + case 'then': + if (target.then) { + // Use a cached value + return target.then; + } + 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: ClientReference = + registerClientReferenceImpl(({}: any), target.$$id, true); + const proxy = new Proxy(clientReference, proxyHandlers); + + // Treat this as a resolved Promise for React's use() + target.status = 'fulfilled'; + target.value = proxy; + + const then = (target.then = registerClientReferenceImpl( + (function then(resolve, reject: any) { + // Expose to React. + return Promise.resolve(resolve(proxy)); + }: any), + // 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. + target.$$id + '#then', + false, + )); + return then; + } else { + // Since typeof .then === 'function' is a feature test we'd continue recursing + // indefinitely if we return a function. Instead, we return an object reference + // if we check further. + return undefined; + } + } + if (typeof name === 'symbol') { + throw new Error( + 'Cannot read Symbol exports. Only named exports are supported on a client module ' + + 'imported on the server.', + ); + } + let cachedReference = target[name]; + if (!cachedReference) { + const reference: ClientReference = registerClientReferenceImpl( + (function () { + throw new Error( + // eslint-disable-next-line react-internal/safe-string-coercion + `Attempted to call ${String(name)}() from the server but ${String( + name, + )} is on the client. ` + + `It's not possible to invoke a client function from the server, it can ` + + `only be rendered as a Component or passed to props of a Client Component.`, + ); + }: any), + target.$$id + '#' + name, + target.$$async, + ); + Object.defineProperty((reference: any), 'name', {value: name}); + cachedReference = target[name] = new Proxy(reference, deepProxyHandlers); + } + return cachedReference; +} + +const proxyHandlers = { + get: function ( + target: Function, + name: string | symbol, + receiver: Proxy, + ): $FlowFixMe { + return getReference(target, name); + }, + getOwnPropertyDescriptor: function ( + target: Function, + name: string | symbol, + ): $FlowFixMe { + let descriptor = Object.getOwnPropertyDescriptor(target, name); + if (!descriptor) { + descriptor = { + value: getReference(target, name), + writable: false, + configurable: false, + enumerable: false, + }; + Object.defineProperty(target, name, descriptor); } - return cachedReference; + return descriptor; }, getPrototypeOf(target: Function): Object { // Pretend to be a Promise in case anyone asks. diff --git a/packages/react-server-dom-webpack/src/ReactFlightWebpackReferences.js b/packages/react-server-dom-webpack/src/ReactFlightWebpackReferences.js index b174953a72c4f..f68c6f27f9640 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightWebpackReferences.js +++ b/packages/react-server-dom-webpack/src/ReactFlightWebpackReferences.js @@ -92,7 +92,11 @@ export function registerServerReference( const PROMISE_PROTOTYPE = Promise.prototype; const deepProxyHandlers = { - get: function (target: Function, name: string, receiver: Proxy) { + get: function ( + target: Function, + name: string | symbol, + receiver: Proxy, + ) { switch (name) { // These names are read by the Flight runtime if you end up using the exports object. case '$$typeof': @@ -117,6 +121,9 @@ const deepProxyHandlers = { case Symbol.toPrimitive: // $FlowFixMe[prop-missing] return Object.prototype[Symbol.toPrimitive]; + case Symbol.toStringTag: + // $FlowFixMe[prop-missing] + return Object.prototype[Symbol.toStringTag]; case 'Provider': throw new Error( `Cannot render a Client Context Provider on the Server. ` + @@ -137,7 +144,7 @@ const deepProxyHandlers = { }, }; -function getReference(target: Function, name: string): $FlowFixMe { +function getReference(target: Function, name: string | symbol): $FlowFixMe { switch (name) { // These names are read by the Flight runtime if you end up using the exports object. case '$$typeof': @@ -158,6 +165,9 @@ function getReference(target: Function, name: string): $FlowFixMe { case Symbol.toPrimitive: // $FlowFixMe[prop-missing] return Object.prototype[Symbol.toPrimitive]; + case Symbol.toStringTag: + // $FlowFixMe[prop-missing] + return Object.prototype[Symbol.toStringTag]; 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. @@ -211,6 +221,12 @@ function getReference(target: Function, name: string): $FlowFixMe { return undefined; } } + if (typeof name === 'symbol') { + throw new Error( + 'Cannot read Symbol exports. Only named exports are supported on a client module ' + + 'imported on the server.', + ); + } let cachedReference = target[name]; if (!cachedReference) { const reference: ClientReference = registerClientReferenceImpl( @@ -236,14 +252,14 @@ function getReference(target: Function, name: string): $FlowFixMe { const proxyHandlers = { get: function ( target: Function, - name: string, + name: string | symbol, receiver: Proxy, ): $FlowFixMe { return getReference(target, name); }, getOwnPropertyDescriptor: function ( target: Function, - name: string, + name: string | symbol, ): $FlowFixMe { let descriptor = Object.getOwnPropertyDescriptor(target, name); if (!descriptor) { diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js index 465123f825500..e336e2ed8cf90 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -568,6 +568,32 @@ describe('ReactFlightDOM', () => { ); }); + it('throws when accessing a symbol prop from client exports', () => { + const symbol = Symbol('test'); + const ClientModule = clientExports({ + Component: {deep: 'thing'}, + }); + function read() { + return ClientModule[symbol]; + } + expect(read).toThrowError( + 'Cannot read Symbol exports. ' + + 'Only named exports are supported on a client module imported on the server.', + ); + }); + + it('does not throw when toString:ing client exports', () => { + const ClientModule = clientExports({ + Component: {deep: 'thing'}, + }); + expect(Object.prototype.toString.call(ClientModule)).toBe( + '[object Object]', + ); + expect(Object.prototype.toString.call(ClientModule.Component)).toBe( + '[object Function]', + ); + }); + it('does not throw when React inspects any deep props', () => { const ClientModule = clientExports({ Component: function () {}, @@ -1655,4 +1681,45 @@ describe('ReactFlightDOM', () => { await collectHints(readable); expect(hintRows.length).toEqual(6); }); + + it('should be able to include a client reference in printed errors', async () => { + const reportedErrors = []; + + const ClientComponent = clientExports(function ({prop}) { + return 'This should never render'; + }); + + const ClientReference = clientExports({}); + + class InvalidValue {} + + const {writable} = getTestStream(); + const {pipe} = ReactServerDOMServer.renderToPipeableStream( +
+ +
, + webpackMap, + { + onError(x) { + reportedErrors.push(x); + }, + }, + ); + pipe(writable); + + expect(reportedErrors.length).toBe(1); + if (__DEV__) { + expect(reportedErrors[0].message).toEqual( + 'Functions cannot be passed directly to Client Components unless you explicitly expose it by marking it with "use server".\n' + + ' <... prop={client} invalid={function}>\n' + + ' ^^^^^^^^^^', + ); + } else { + expect(reportedErrors[0].message).toEqual( + 'Functions cannot be passed directly to Client Components unless you explicitly expose it by marking it with "use server".\n' + + ' {prop: client, invalid: function}\n' + + ' ^^^^^^^^', + ); + } + }); }); diff --git a/packages/shared/ReactSerializationErrors.js b/packages/shared/ReactSerializationErrors.js index e0def3e1c657d..5dbb79a704fc6 100644 --- a/packages/shared/ReactSerializationErrors.js +++ b/packages/shared/ReactSerializationErrors.js @@ -98,6 +98,9 @@ export function describeValueForErrorMessage(value: mixed): string { if (isArray(value)) { return '[...]'; } + if (value !== null && value.$$typeof === CLIENT_REFERENCE_TAG) { + return describeClientReference(value); + } const name = objectName(value); if (name === 'Object') { return '{...}'; @@ -105,6 +108,9 @@ export function describeValueForErrorMessage(value: mixed): string { return name; } case 'function': + if ((value: any).$$typeof === CLIENT_REFERENCE_TAG) { + return describeClientReference(value); + } return 'function'; default: // eslint-disable-next-line react-internal/safe-string-coercion @@ -142,6 +148,12 @@ function describeElementType(type: any): string { return ''; } +const CLIENT_REFERENCE_TAG = Symbol.for('react.client.reference'); + +function describeClientReference(ref: any) { + return 'client'; +} + export function describeObjectForErrorMessage( objectOrArray: {+[key: string | number]: mixed, ...} | $ReadOnlyArray, expandedName?: string, @@ -210,6 +222,8 @@ export function describeObjectForErrorMessage( } else { if (objectOrArray.$$typeof === REACT_ELEMENT_TYPE) { str = '<' + describeElementType(objectOrArray.type) + '/>'; + } else if (objectOrArray.$$typeof === CLIENT_REFERENCE_TAG) { + return describeClientReference(objectOrArray); } else if (__DEV__ && jsxPropsParents.has(objectOrArray)) { // Print JSX const type = jsxPropsParents.get(objectOrArray);