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

[compiler][wip] Inline single return JSX for known function components #30901

Draft
wants to merge 4 commits into
base: gh/josephsavona/56/base
Choose a base branch
from

Conversation

josephsavona
Copy link
Contributor

@josephsavona josephsavona commented Sep 6, 2024

Stack from ghstack (oldest at bottom):

See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

[ghstack-poisoned]
Copy link

vercel bot commented Sep 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ❌ Failed (Inspect) Sep 10, 2024 0:21am

josephsavona added a commit that referenced this pull request Sep 6, 2024
See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

ghstack-source-id: af5118bd706d524412f90c6fc4e30480e8d49b93
Pull Request resolved: #30901
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 6, 2024
@josephsavona josephsavona marked this pull request as draft September 6, 2024 23:09
Comment on lines +334 to +346
/**
* If a given component has a single return statement and returns JSX where the
* tag is known (via moduleTypeProvider) to be a function component, then it is
* safe to convert the JSX expression into a direct call to the component in question:
*
* function Parent() {
* return <Child />
* }
* =>
* function Parent() {
* return Child({})
* }
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the basic idea. We have to know that the tag corresponds to a function component, though, for this transformation to be safe.

function Component({a, b}) {
const c = [a, b];
return (
<Child value={c}>
Copy link
Member

Choose a reason for hiding this comment

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

Btw a deopt case to think about is if Child is passed a reactive key, since this should remount the component when it changes right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, yeah similar to the reactive tag check.

…n components"

See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 6, 2024
See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

ghstack-source-id: 32dfc075e28271b10b3b14611256dbec8a9ac1b9
Pull Request resolved: #30901
} else {
t1 = $[1];
}
return Child({ value: a, children: t1 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you emit:

const result = Child({ value: a, children: t1 });
if (__DEV__ && typeof result === 'object' && result !== null) {
  (result._debugInfo || (result._debugInfo = [])).unshift({
    name: 'Child',
  });
}
return result;

It should show up in DevTools (built from main) as a separate Component. Same as Server Components.

Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 7, 2024

Choose a reason for hiding this comment

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

I'll probably add props too so might as well do:

.unshift({
  name: 'Child',
  props: { value: a, children: t1 }
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ofc, if it's cached it probably shouldn't keep unshifting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think we have to wrap the result in a fragment and attach the debug info there. Bc there could be multiple levels of this inlining happening and we’d attach to the innermost result w the outermost component name.

Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 9, 2024

Choose a reason for hiding this comment

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

Note that RSC already does something similar without a fragment to avoid changing behavior.

This means that we also add this data to non-React types. Mostly an array of children also gets this or a Promise of children. Anything that’s a valid React node object. Maybe we should’ve used a symbol instead for this reason but faster this way.

The return value of an inlined function could itself be a return value from a server component which would also already have some debug info.

That’s why I conditionally added or reused the debug info array in my example.

Note that I also used unshift rather than push to add it to the beginning.

So if the inner one rendered Server Component > div. By adding it to the beginning we ensure the tree becomes Child > Server Component > div.

The same thing works for your example where it is multiple inlined because each level would add to the beginning and the outer one is the last to add it to the beginning so it’s correct.

Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 9, 2024

Choose a reason for hiding this comment

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

Note that wrapping in a keyless fragment is not actually the same because a keyless fragment is the same a no fragment when switching between them. But adding a keyless fragment at the root should change the behavior of a return value that conditionally renderers a keyless fragment or plain jsx. Since it would no longer be at the root.

So best to avoid changing the dev behavior since subtle things like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of mutating it could clone the react element or array from the child and clone the debug info.

Copy link
Contributor Author

@josephsavona josephsavona Sep 10, 2024

Choose a reason for hiding this comment

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

Ah, that makes sense in terms of the unshifting maintaining the stack. One other consideration is hooks. With this change, all hooks from the inlined component will presumably show up as part of the outer component. Thoughts on what to do there? For example, the compiler could emit (in dev) a call to some function to say "we're in component X now" so that subsequent hook calls can be associated to that component.

cc @hoxyq

Copy link
Contributor

@hoxyq hoxyq Sep 12, 2024

Choose a reason for hiding this comment

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

Disclaimer: I might be missing some context here.

This is the simplified algorithm of how hook tree parsing works in DevTools currently:

  1. Get the render function of the inspected component from its Fiber
  2. Patch React Dispatcher so that every built-in hook call will populate a new frame in a custom callstack
  3. Call render function of the component
  4. Parse the callstack and re-create the hook tree

With this change (in this PR), inlined single return JSX won't have a corresponding Fiber, right? Thats why @sebmarkbage is suggesting to patch it with _debugInfo, so that it is displayed as a virtual instance in React DevTools (same as Server Component). Since this is a virtual instance, there is no stateful node (like Fiber) that can be used.

For example, the compiler could emit (in dev) a call to some function to say "we're in component X now"

Parsing hook tree can be expensive, this is why we do this lazily on DevTools side, only when user selects some component. We could potentially add some API on the RDT global hook, like this:

__REACT_DEVTOOLS_GLOBAL_HOOK__.registerComponentEntry(...)

Which will be no-op if user didn't select any component in RDT yet. Once user selects some component, RDT will:

  1. Patch this method with a real implementation
  2. Find lowest non-virtual parent instance (with a Fiber) for this component
  3. Call render function on this component
  4. Track hook calls and match them with the component entries

Thats just an idea of how this might look like, maybe there are better ways to do this, but we definitely should do this lazily on DevTools side

…n components"

See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 10, 2024
See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

ghstack-source-id: 07e37882fbe2aff5728aec50aef40ede66bf3d55
Pull Request resolved: #30901
…n components"

See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 10, 2024
See comments, still WIP bc i'm getting a weird error with Babel and line numbers, one of the source locations must be off.

ghstack-source-id: 23e2ff3a6b9a57b15f15adc4b4ee9a5ddaad98f5
Pull Request resolved: #30901
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants