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

Track Stack of JSX Calls #29032

Merged
merged 2 commits into from
May 9, 2024
+483 −70
Merged

Track Stack of JSX Calls #29032

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
a2470c8
Add Feature Flag
sebmarkbage May 7, 2024
80f37ab
Track stacks in JSX
sebmarkbage May 7, 2024
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
20 changes: 20 additions & 0 deletions packages/react-client/src/ReactFlightClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
enablePostpone,
enableRefAsProp,
enableFlightReadableStream,
enableOwnerStacks,
} from 'shared/ReactFeatureFlags';

import {
Expand Down Expand Up @@ -563,6 +564,7 @@ function createElement(
key: mixed,
props: mixed,
owner: null | ReactComponentInfo, // DEV-only
stack: null | string, // DEV-only
): React$Element<any> {
let element: any;
if (__DEV__ && enableRefAsProp) {
Expand Down Expand Up @@ -623,6 +625,23 @@ function createElement(
writable: true,
value: null,
});
if (enableOwnerStacks) {
Object.defineProperty(element, '_debugStack', {
configurable: false,
enumerable: false,
writable: true,
value: {stack: stack},
});
Object.defineProperty(element, '_debugTask', {
configurable: false,
enumerable: false,
writable: true,
value: null,
});
}
// TODO: We should be freezing the element but currently, we might write into
// _debugInfo later. We could move it into _store which remains mutable.
Object.freeze(element.props);
Copy link
Collaborator

Choose a reason for hiding this comment

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

On a Next.js client side navigations, we get the following RSC payload:

3:I["(app-pages-browser)/../../../../packages/next/dist/client/components/client-page.js",["app-pages-internals","static/chunks/app-pages-internals.js"],"ClientPageRoot"]
4:I["(app-pages-browser)/./app/[id]/page.tsx",["app/[id]/page","static/chunks/app/%5Bid%5D/page.js"],"default",1]
5:I["(app-pages-browser)/../../../../packages/next/dist/client/components/layout-router.js",["app-pages-internals","static/chunks/app-pages-internals.js"],""]
6:I["(app-pages-browser)/../../../../packages/next/dist/client/components/render-from-template-context.js",["app-pages-internals","static/chunks/app-pages-internals.js"],""]
2:{"name":"","env":"Server","owner":null}
1:D"$2"
8:{"name":"","env":"Server","owner":null}
7:D"$8"
0:["development",[["children",["id","a","d"],[["id","a","d"],{"children":["__PAGE__",{}]}],[["id","a","d"],{"children":["__PAGE__",{},[["$L1",["$","$L3",null,{"props":{"params":{"id":"a"},"searchParams":{}},"Component":"$4"},null]],null],null]},["$","$L5",null,{"parallelRouterKey":"children","segmentPath":["children","$0:1:0:3:0","children"],"error":"$undefined","errorStyles":"$undefined","errorScripts":"$undefined","template":["$","$L6",null,{},null],"templateStyles":"$undefined","templateScripts":"$undefined","notFound":"$undefined","notFoundStyles":"$undefined","styles":null},null],null],[null,"$L7"]]]]
9:"$Sreact.fragment"
7:["$","$9","19VxPb-E-LbHeAfz9ZCJ2",{"children":[["$","meta","0",{"name":"viewport","content":"width=device-width, initial-scale=1"},null],["$","meta","1",{"charSet":"utf-8"},null]]}]
1:null

Which then results in ""Cannot assign to read only property 'Component' of object '#'" thrown from

react/packages/react-client/src/ReactFlightClient.js

Line 644 in 6c409ac

Object.freeze(element.props);

I don't have a smaller repro yet. Is this a bug with the Flight Server, our usage or should we just not freeze the props on the Flight Client?

Copy link

@hi-ogawa hi-ogawa May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think I'm seeing a similar issue here hi-ogawa/vite-plugins#318 I don't have a repro outside of my framework yet, but the error seems related to passing client reference as client component props like below:

  • page.tsx
import { Client1, Client2 } from "./_client";

export default function Page() {
  return (
    <div>
      <h4 className="font-bold">Repro</h4>
      {/* TypeError: Cannot assign to read only property 'SomeComp' of object '#<Object>' */}
      <Client1 SomeComp={Client2} />
    </div>
  );
}
  • _client.tsx
"use client";

import React from "react";

export function Client1(props: { SomeComp: React.ComponentType }) {
  return (
    <div>
      [Client1] props.SomeComp = <props.SomeComp />
    </div>
  );
}

export function Client2() {
  return <span>[Client2]</span>;
}

I'm using a similar pattern for Error boundary and flight client seems to be throwing for this.

}
return element;
}
Expand Down Expand Up @@ -1003,6 +1022,7 @@ function parseModelTuple(
tuple[2],
tuple[3],
__DEV__ ? (tuple: any)[4] : null,
__DEV__ && enableOwnerStacks ? (tuple: any)[5] : null,
);
}
return value;
Expand Down
151 changes: 127 additions & 24 deletions packages/react-client/src/__tests__/ReactFlight-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,24 @@ if (typeof File === 'undefined' || typeof FormData === 'undefined') {
function normalizeCodeLocInfo(str) {
return (
str &&
str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function (m, name) {
return '\n in ' + name + (/\d/.test(m) ? ' (at **)' : '');
str.replace(/^ +(?:at|in) ([\S]+)[^\n]*/gm, function (m, name) {
return ' in ' + name + (/\d/.test(m) ? ' (at **)' : '');
})
);
}

function getDebugInfo(obj) {
const debugInfo = obj._debugInfo;
if (debugInfo) {
for (let i = 0; i < debugInfo.length; i++) {
if (typeof debugInfo[i].stack === 'string') {
debugInfo[i].stack = normalizeCodeLocInfo(debugInfo[i].stack);
}
}
}
return debugInfo;
}

const heldValues = [];
let finalizationCallback;
function FinalizationRegistryMock(callback) {
Expand Down Expand Up @@ -221,8 +233,19 @@ describe('ReactFlight', () => {
await act(async () => {
const rootModel = await ReactNoopFlightClient.read(transport);
const greeting = rootModel.greeting;
expect(greeting._debugInfo).toEqual(
__DEV__ ? [{name: 'Greeting', env: 'Server', owner: null}] : undefined,
expect(getDebugInfo(greeting)).toEqual(
__DEV__
? [
{
name: 'Greeting',
env: 'Server',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
},
]
: undefined,
);
ReactNoop.render(greeting);
});
Expand All @@ -248,8 +271,19 @@ describe('ReactFlight', () => {

await act(async () => {
const promise = ReactNoopFlightClient.read(transport);
expect(promise._debugInfo).toEqual(
__DEV__ ? [{name: 'Greeting', env: 'Server', owner: null}] : undefined,
expect(getDebugInfo(promise)).toEqual(
__DEV__
? [
{
name: 'Greeting',
env: 'Server',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
},
]
: undefined,
);
ReactNoop.render(await promise);
});
Expand Down Expand Up @@ -2233,9 +2267,11 @@ describe('ReactFlight', () => {
return <span>!</span>;
}

const lazy = React.lazy(async () => ({
default: <ThirdPartyLazyComponent />,
}));
const lazy = React.lazy(async function myLazy() {
return {
default: <ThirdPartyLazyComponent />,
};
});

function ThirdPartyComponent() {
return <span>stranger</span>;
Expand Down Expand Up @@ -2269,31 +2305,61 @@ describe('ReactFlight', () => {

await act(async () => {
const promise = ReactNoopFlightClient.read(transport);
expect(promise._debugInfo).toEqual(
expect(getDebugInfo(promise)).toEqual(
__DEV__
? [{name: 'ServerComponent', env: 'Server', owner: null}]
? [
{
name: 'ServerComponent',
env: 'Server',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
},
]
: undefined,
);
const result = await promise;
const thirdPartyChildren = await result.props.children[1];
// We expect the debug info to be transferred from the inner stream to the outer.
expect(thirdPartyChildren[0]._debugInfo).toEqual(
expect(getDebugInfo(thirdPartyChildren[0])).toEqual(
__DEV__
? [{name: 'ThirdPartyComponent', env: 'third-party', owner: null}]
? [
{
name: 'ThirdPartyComponent',
env: 'third-party',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
},
]
: undefined,
);
expect(thirdPartyChildren[1]._debugInfo).toEqual(
expect(getDebugInfo(thirdPartyChildren[1])).toEqual(
__DEV__
? [{name: 'ThirdPartyLazyComponent', env: 'third-party', owner: null}]
? [
{
name: 'ThirdPartyLazyComponent',
env: 'third-party',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in myLazy (at **)\n in lazyInitializer (at **)'
: undefined,
},
]
: undefined,
);
expect(thirdPartyChildren[2]._debugInfo).toEqual(
expect(getDebugInfo(thirdPartyChildren[2])).toEqual(
__DEV__
? [
{
name: 'ThirdPartyFragmentComponent',
env: 'third-party',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
},
]
: undefined,
Expand Down Expand Up @@ -2357,24 +2423,47 @@ describe('ReactFlight', () => {

await act(async () => {
const promise = ReactNoopFlightClient.read(transport);
expect(promise._debugInfo).toEqual(
expect(getDebugInfo(promise)).toEqual(
__DEV__
? [{name: 'ServerComponent', env: 'Server', owner: null}]
? [
{
name: 'ServerComponent',
env: 'Server',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
},
]
: undefined,
);
const result = await promise;
const thirdPartyFragment = await result.props.children;
expect(thirdPartyFragment._debugInfo).toEqual(
__DEV__ ? [{name: 'Keyed', env: 'Server', owner: null}] : undefined,
expect(getDebugInfo(thirdPartyFragment)).toEqual(
__DEV__
? [
{
name: 'Keyed',
env: 'Server',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in ServerComponent (at **)'
: undefined,
},
]
: undefined,
);
// We expect the debug info to be transferred from the inner stream to the outer.
expect(thirdPartyFragment.props.children._debugInfo).toEqual(
expect(getDebugInfo(thirdPartyFragment.props.children)).toEqual(
__DEV__
? [
{
name: 'ThirdPartyAsyncIterableComponent',
env: 'third-party',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
},
]
: undefined,
Expand Down Expand Up @@ -2467,10 +2556,24 @@ describe('ReactFlight', () => {
// We've rendered down to the span.
expect(greeting.type).toBe('span');
if (__DEV__) {
const greetInfo = {name: 'Greeting', env: 'Server', owner: null};
expect(greeting._debugInfo).toEqual([
const greetInfo = {
name: 'Greeting',
env: 'Server',
owner: null,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Object.<anonymous> (at **)'
: undefined,
};
expect(getDebugInfo(greeting)).toEqual([
greetInfo,
{name: 'Container', env: 'Server', owner: greetInfo},
{
name: 'Container',
env: 'Server',
owner: greetInfo,
stack: gate(flag => flag.enableOwnerStacks)
? ' in Greeting (at **)'
: undefined,
},
]);
// The owner that created the span was the outer server component.
// We expect the debug info to be referentially equal to the owner.
Expand Down
18 changes: 13 additions & 5 deletions packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ describe('ReactFlightDOMEdge', () => {

const serializedContent = await readResult(stream1);

expect(serializedContent.length).toBeLessThan(400);
expect(serializedContent.length).toBeLessThan(410);
expect(timesRendered).toBeLessThan(5);

const model = await ReactServerDOMClient.createFromReadableStream(stream2, {
Expand Down Expand Up @@ -296,7 +296,7 @@ describe('ReactFlightDOMEdge', () => {
const [stream1, stream2] = passThrough(stream).tee();

const serializedContent = await readResult(stream1);
expect(serializedContent.length).toBeLessThan(400);
expect(serializedContent.length).toBeLessThan(__DEV__ ? 590 : 400);
expect(timesRendered).toBeLessThan(5);

const model = await ReactServerDOMClient.createFromReadableStream(stream2, {
Expand Down Expand Up @@ -324,7 +324,7 @@ describe('ReactFlightDOMEdge', () => {
<ServerComponent recurse={20} />,
);
const serializedContent = await readResult(stream);
const expectedDebugInfoSize = __DEV__ ? 64 * 20 : 0;
const expectedDebugInfoSize = __DEV__ ? 300 * 20 : 0;
expect(serializedContent.length).toBeLessThan(150 + expectedDebugInfoSize);
});

Expand Down Expand Up @@ -742,10 +742,18 @@ describe('ReactFlightDOMEdge', () => {
// We've rendered down to the span.
expect(greeting.type).toBe('span');
if (__DEV__) {
const greetInfo = {name: 'Greeting', env: 'Server', owner: null};
const greetInfo = expect.objectContaining({
name: 'Greeting',
env: 'Server',
owner: null,
});
expect(lazyWrapper._debugInfo).toEqual([
greetInfo,
{name: 'Container', env: 'Server', owner: greetInfo},
expect.objectContaining({
name: 'Container',
env: 'Server',
owner: greetInfo,
}),
]);
// The owner that created the span was the outer server component.
// We expect the debug info to be referentially equal to the owner.
Expand Down
Loading