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

useId: Remove unnecessary try/finally blocks #27340

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 6, 2023

To generate IDs for useId, we modify a context variable whenever multiple siblings are rendered, or when a component includes a useId hook.

When this happens, we must ensure that the context is reset properly on unwind if something errors or suspends. When I originally implemented this, I did this by wrapping the child's rendering with a try/finally block. But a better way to do this is by using the non-destructive renderNode path instead of renderNodeDestructive.

To generate IDs for useId, we modify a context variable whenever multiple
siblings are rendered, or when a component includes a useId hook.

When this happens, we must ensure that the context is reset properly on unwind
if something errors or suspends. When I originally implemented this, I did this
by wrapping the child's rendering with a try/finally block. But a better way to
do this is by using the non-destructive renderNode path instead of
renderNodeDestructive.
@acdlite acdlite requested a review from sebmarkbage September 6, 2023 19:27
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 6, 2023
@react-sizebot
Copy link

Comparing: b9be453...d0d6924

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.70 kB 174.70 kB = 54.61 kB 54.61 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.development.js +0.47% 149.63 kB 150.33 kB +0.45% 37.26 kB 37.43 kB
oss-stable/react-server/cjs/react-server.development.js +0.47% 149.63 kB 150.33 kB +0.45% 37.26 kB 37.43 kB
oss-experimental/react-server/cjs/react-server.development.js +0.47% 160.56 kB 161.31 kB +0.40% 39.96 kB 40.12 kB
oss-experimental/react-dom/cjs/react-dom-server.bun.development.js +0.21% 362.12 kB 362.87 kB +0.20% 81.20 kB 81.36 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.21% 364.68 kB 365.42 kB +0.20% 81.68 kB 81.85 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js +0.20% 366.59 kB 367.33 kB +0.20% 82.14 kB 82.30 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +0.20% 368.70 kB 369.45 kB +0.20% 82.23 kB 82.39 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.development.js +0.20% 369.11 kB 369.86 kB +0.20% 82.35 kB 82.51 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +0.20% 370.00 kB 370.75 kB +0.20% 82.55 kB 82.71 kB
oss-stable-semver/react-dom/cjs/react-dom-server.bun.development.js +0.20% 345.51 kB 346.21 kB +0.23% 77.57 kB 77.75 kB
oss-stable/react-dom/cjs/react-dom-server.bun.development.js +0.20% 345.54 kB 346.24 kB +0.23% 77.60 kB 77.78 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.20% 348.07 kB 348.77 kB +0.22% 78.06 kB 78.23 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.20% 348.10 kB 348.80 kB +0.22% 78.08 kB 78.25 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.development.js +0.20% 348.29 kB 348.99 kB +0.22% 78.50 kB 78.67 kB
oss-stable/react-dom/cjs/react-dom-server.browser.development.js +0.20% 348.32 kB 349.01 kB +0.22% 78.52 kB 78.69 kB
oss-stable-semver/react-dom/cjs/react-dom-server.edge.development.js +0.20% 348.70 kB 349.40 kB +0.23% 78.61 kB 78.79 kB
oss-stable/react-dom/cjs/react-dom-server.edge.development.js +0.20% 348.73 kB 349.42 kB +0.23% 78.64 kB 78.82 kB

Generated by 🚫 dangerJS against d0d6924

@acdlite acdlite merged commit ee7f9c9 into facebook:main Sep 6, 2023
github-actions bot pushed a commit that referenced this pull request Sep 6, 2023
To generate IDs for useId, we modify a context variable whenever
multiple siblings are rendered, or when a component includes a useId
hook.

When this happens, we must ensure that the context is reset properly on
unwind if something errors or suspends. When I originally implemented
this, I did this by wrapping the child's rendering with a try/finally
block. But a better way to do this is by using the non-destructive
renderNode path instead of renderNodeDestructive.

DiffTrain build for [ee7f9c9](ee7f9c9)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
To generate IDs for useId, we modify a context variable whenever
multiple siblings are rendered, or when a component includes a useId
hook.

When this happens, we must ensure that the context is reset properly on
unwind if something errors or suspends. When I originally implemented
this, I did this by wrapping the child's rendering with a try/finally
block. But a better way to do this is by using the non-destructive
renderNode path instead of renderNodeDestructive.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
To generate IDs for useId, we modify a context variable whenever
multiple siblings are rendered, or when a component includes a useId
hook.

When this happens, we must ensure that the context is reset properly on
unwind if something errors or suspends. When I originally implemented
this, I did this by wrapping the child's rendering with a try/finally
block. But a better way to do this is by using the non-destructive
renderNode path instead of renderNodeDestructive.

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