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

Encode lazy Node slots properly in key path and resumable paths #27359

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Sep 11, 2023

It's possible to postpone a specific node and not using a wrapper component. Therefore we encode the resumable slot as the index slot. When it's a plain client component that postpones, it's encoded as the child slot inside that component which is the one that's postponed rather than the component itself.

Since it's possible for a child slot to suspend (e.g. React.lazy's microtask in this case) retryTask might need to keep its index around when it resolves.

@sebmarkbage sebmarkbage requested a review from acdlite September 11, 2023 22:07
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 11, 2023
@@ -493,6 +493,46 @@ describe('ReactDOMFizzStaticBrowser', () => {
// TODO: expect(getVisibleChildren(container)).toEqual(<div>Hello</div>);
});

it('supports postponing in lazy in prerender and resuming later', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
it('supports postponing in lazy in prerender and resuming later', async () => {
// @gate enablePostpone
it('supports postponing in lazy in prerender and resuming later', async () => {

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Sep 11, 2023

When it's a plain client component that postpones, it's encoded as the child slot inside that component which is the one that's postponed rather than the component itself.

I'm not sure this is 100% correct either. If lazy() resolves to an array then those are a nested array inside the slot which is correct. However, if a component resolves to an array then those represents multiple slots in the component and not just the zero slot. Might need two different representations to separate these.

Actually, it's correct. A lazy() that resolves to an array in the child of a component should be equivalent to that being an array in the first place which wouldn't have the nested slots. So both cases should be equivalent.

It's possible to postpone a specific node and not using a wrapper component.
Therefore we encode the resumable slot as the index slot. When it's
a plain client component that postpones, it's encoded as the child slot
inside that component which is the one that's postponed rather than the
component itself.

Since it's possible for a child slot to suspend (e.g. React.lazy's microtask
in this case) retryTask might need to keep its index around when it resolves.
@react-sizebot
Copy link

react-sizebot commented Sep 11, 2023

Comparing: 627b7ab...4c17531

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 165.63 kB 165.63 kB = 51.88 kB 51.88 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 174.82 kB 174.82 kB = 54.69 kB 54.69 kB
facebook-www/ReactDOM-prod.classic.js = 570.44 kB 570.44 kB = 100.45 kB 100.45 kB
facebook-www/ReactDOM-prod.modern.js = 554.21 kB 554.21 kB = 97.61 kB 97.61 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-server/cjs/react-server.production.min.js +0.86% 26.99 kB 27.22 kB +0.81% 9.03 kB 9.10 kB
oss-stable/react-server/cjs/react-server.production.min.js +0.86% 26.99 kB 27.22 kB +0.81% 9.03 kB 9.10 kB
oss-experimental/react-server/cjs/react-server.production.min.js +0.75% 29.08 kB 29.29 kB +0.56% 9.63 kB 9.68 kB
oss-stable-semver/react-server/cjs/react-server.development.js +0.51% 154.43 kB 155.23 kB +0.32% 38.02 kB 38.14 kB
oss-stable/react-server/cjs/react-server.development.js +0.51% 154.43 kB 155.23 kB +0.32% 38.02 kB 38.14 kB
oss-experimental/react-server/cjs/react-server.development.js +0.49% 165.62 kB 166.44 kB +0.23% 40.80 kB 40.89 kB
facebook-www/ReactDOMServer-prod.modern.js +0.44% 156.25 kB 156.95 kB +0.40% 28.39 kB 28.50 kB
facebook-www/ReactDOMServer-prod.classic.js +0.44% 157.15 kB 157.85 kB +0.37% 28.64 kB 28.74 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js +0.44% 158.40 kB 159.09 kB +0.38% 29.36 kB 29.47 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.37% 63.10 kB 63.33 kB +0.29% 19.23 kB 19.29 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.37% 63.12 kB 63.35 kB +0.29% 19.26 kB 19.31 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.37% 62.94 kB 63.17 kB +0.33% 18.90 kB 18.97 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.37% 62.96 kB 63.19 kB +0.33% 18.93 kB 18.99 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.production.min.js +0.36% 63.91 kB 64.14 kB +0.32% 19.81 kB 19.87 kB
oss-stable/react-dom/cjs/react-dom-server.browser.production.min.js +0.36% 63.93 kB 64.16 kB +0.33% 19.83 kB 19.90 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.production.min.js +0.36% 64.05 kB 64.28 kB +0.23% 20.10 kB 20.14 kB
oss-stable/react-dom/umd/react-dom-server.browser.production.min.js +0.36% 64.08 kB 64.31 kB +0.23% 20.12 kB 20.17 kB
oss-stable-semver/react-dom/cjs/react-dom-server.bun.production.min.js +0.35% 66.24 kB 66.47 kB +0.31% 20.22 kB 20.29 kB
oss-stable/react-dom/cjs/react-dom-server.bun.production.min.js +0.35% 66.26 kB 66.49 kB +0.31% 20.25 kB 20.31 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.35% 66.47 kB 66.70 kB +0.42% 20.42 kB 20.51 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.35% 66.32 kB 66.55 kB +0.27% 20.03 kB 20.08 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.34% 67.82 kB 68.05 kB +0.34% 20.52 kB 20.59 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.34% 67.84 kB 68.08 kB +0.33% 20.54 kB 20.61 kB
oss-stable-semver/react-dom/cjs/react-dom-server.edge.production.min.js +0.34% 68.14 kB 68.37 kB +0.33% 21.24 kB 21.31 kB
oss-stable/react-dom/cjs/react-dom-server.edge.production.min.js +0.34% 68.17 kB 68.40 kB +0.34% 21.27 kB 21.34 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.production.min.js +0.34% 68.20 kB 68.43 kB +0.39% 21.24 kB 21.33 kB
oss-stable/react-dom/cjs/react-dom-server.node.production.min.js +0.34% 68.22 kB 68.46 kB +0.39% 21.27 kB 21.35 kB
oss-experimental/react-dom/cjs/react-dom-server.bun.production.min.js +0.33% 69.87 kB 70.10 kB +0.20% 21.42 kB 21.47 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js +0.33% 69.19 kB 69.41 kB +0.30% 21.11 kB 21.18 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js +0.32% 69.32 kB 69.54 kB +0.28% 21.40 kB 21.45 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.32% 71.45 kB 71.68 kB +0.27% 21.69 kB 21.75 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +0.31% 73.92 kB 74.14 kB +0.21% 22.81 kB 22.85 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.production.min.js +0.31% 73.64 kB 73.87 kB +0.27% 22.62 kB 22.68 kB
facebook-www/ReactDOMServerStreaming-dev.modern.js +0.23% 356.08 kB 356.91 kB +0.21% 78.83 kB 78.99 kB
facebook-www/ReactDOMServer-dev.modern.js +0.23% 360.88 kB 361.71 kB +0.21% 79.99 kB 80.15 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.development.js +0.23% 368.06 kB 368.91 kB +0.14% 79.71 kB 79.82 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.development.js +0.23% 368.09 kB 368.93 kB +0.14% 79.74 kB 79.85 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.development.js +0.23% 368.28 kB 369.13 kB +0.14% 80.19 kB 80.30 kB
oss-stable/react-dom/umd/react-dom-server.browser.development.js +0.23% 368.31 kB 369.15 kB +0.14% 80.22 kB 80.33 kB
oss-stable-semver/react-dom/cjs/react-dom-server.bun.development.js +0.23% 348.66 kB 349.45 kB +0.16% 78.36 kB 78.48 kB
oss-stable/react-dom/cjs/react-dom-server.bun.development.js +0.23% 348.69 kB 349.48 kB +0.15% 78.39 kB 78.51 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.23% 351.22 kB 352.01 kB +0.15% 78.85 kB 78.97 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.23% 351.24 kB 352.04 kB +0.16% 78.87 kB 79.00 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.development.js +0.23% 351.44 kB 352.23 kB +0.16% 79.29 kB 79.41 kB
oss-stable/react-dom/cjs/react-dom-server.browser.development.js +0.23% 351.46 kB 352.26 kB +0.16% 79.32 kB 79.45 kB
oss-stable-semver/react-dom/cjs/react-dom-server.edge.development.js +0.23% 351.85 kB 352.64 kB +0.16% 79.42 kB 79.54 kB
oss-stable/react-dom/cjs/react-dom-server.edge.development.js +0.23% 351.87 kB 352.67 kB +0.16% 79.45 kB 79.57 kB
facebook-www/ReactDOMServer-dev.classic.js +0.23% 368.31 kB 369.14 kB +0.21% 81.64 kB 81.81 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.development.js +0.23% 352.91 kB 353.71 kB +0.15% 79.33 kB 79.45 kB
oss-stable/react-dom/cjs/react-dom-server.node.development.js +0.23% 352.94 kB 353.74 kB +0.15% 79.36 kB 79.48 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.development.js +0.23% 353.13 kB 353.92 kB +0.15% 79.31 kB 79.43 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.development.js +0.23% 353.15 kB 353.95 kB +0.16% 79.33 kB 79.45 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js +0.22% 385.61 kB 386.47 kB +0.12% 83.37 kB 83.47 kB
oss-experimental/react-dom/cjs/react-dom-server.bun.development.js +0.22% 365.53 kB 366.34 kB +0.11% 82.03 kB 82.12 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.22% 368.08 kB 368.90 kB +0.11% 82.51 kB 82.60 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js +0.22% 369.99 kB 370.81 kB +0.11% 82.96 kB 83.05 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +0.22% 391.95 kB 392.81 kB +0.12% 84.09 kB 84.19 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +0.22% 374.13 kB 374.95 kB +0.11% 83.19 kB 83.28 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.development.js +0.22% 374.54 kB 375.36 kB +0.11% 83.32 kB 83.41 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +0.22% 375.43 kB 376.25 kB +0.11% 83.51 kB 83.60 kB

Generated by 🚫 dangerJS against 4c17531

Since we know that we're not going to suspend again (we asssume the
iterator doesn't) and we know we're about to call renderChildrenArray
again. We can just do that directly instead of going through renderNode
another time.

This also helps separate these two paths a bit so it can have different
logic.
@sebmarkbage sebmarkbage merged commit bb1d8d1 into facebook:main Sep 11, 2023
github-actions bot pushed a commit that referenced this pull request Sep 11, 2023
It's possible to postpone a specific node and not using a wrapper
component. Therefore we encode the resumable slot as the index slot.
When it's a plain client component that postpones, it's encoded as the
child slot inside that component which is the one that's postponed
rather than the component itself.

Since it's possible for a child slot to suspend (e.g. React.lazy's
microtask in this case) retryTask might need to keep its index around
when it resolves.

DiffTrain build for [bb1d8d1](bb1d8d1)
@sebmarkbage
Copy link
Collaborator Author

This is still not quite right. If you have a React.lazy or Usable inside a nested array it should still have the extra key path with extra slot before it. The isArray check isn't enough to determine if it's nested. This means this information must also survive spawning a task.

This also is relevant to the resuming path because it's different whether the resuming happens at a point that can return a root array or resumes in the slot inside the root array.

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…book#27359)

It's possible to postpone a specific node and not using a wrapper
component. Therefore we encode the resumable slot as the index slot.
When it's a plain client component that postpones, it's encoded as the
child slot inside that component which is the one that's postponed
rather than the component itself.

Since it's possible for a child slot to suspend (e.g. React.lazy's
microtask in this case) retryTask might need to keep its index around
when it resolves.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
It's possible to postpone a specific node and not using a wrapper
component. Therefore we encode the resumable slot as the index slot.
When it's a plain client component that postpones, it's encoded as the
child slot inside that component which is the one that's postponed
rather than the component itself.

Since it's possible for a child slot to suspend (e.g. React.lazy's
microtask in this case) retryTask might need to keep its index around
when it resolves.

DiffTrain build for commit bb1d8d1.
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.

4 participants