Skip to content

useId: Add regression test for bottom-up passing #22675

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

Closed

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Nov 1, 2021

Summary

Passing test for #20127. Was failing with useOpaqueIdentifier.

The useId tests didn't have any test with passing the value to another component via useEffect. Im not particulary bought into this specific pattern. Mostly filing this to get clarification whether this pattern is generally supported or not.

Closes #20127

How did you test this change?

  • jest

@eps1lon eps1lon changed the title test(useOpaqueIdentifier): Add failing test for bottom-up passing useId: Add failing test for bottom-up passing Nov 1, 2021
@sizebot
Copy link

sizebot commented Nov 1, 2021

Comparing: 8c4a05b...cbbf54a

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 = 130.01 kB 130.01 kB = 41.55 kB 41.55 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.99 kB 134.99 kB = 43.02 kB 43.02 kB
facebook-www/ReactDOM-prod.classic.js = 424.29 kB 424.29 kB = 78.28 kB 78.28 kB
facebook-www/ReactDOM-prod.modern.js = 412.85 kB 412.85 kB = 76.51 kB 76.51 kB
facebook-www/ReactDOMForked-prod.classic.js = 424.29 kB 424.29 kB = 78.28 kB 78.28 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against cbbf54a

@acdlite
Copy link
Collaborator

acdlite commented Nov 1, 2021

So this pattern isn't great for other reasons, but they're not anything to with useId itself.

The string returned from useId doesn't have any special restrictions attached to it. It's the same as a string returned from, say, useState or something, except you can't update it and React decides what the value is.

@acdlite acdlite changed the title useId: Add failing test for bottom-up passing useId: Add regression test for bottom-up passing Nov 1, 2021
@eps1lon eps1lon force-pushed the test/opaque-identifier-two-way branch from 725e012 to cbbf54a Compare November 1, 2021 22:20
@eps1lon
Copy link
Collaborator Author

eps1lon commented Nov 1, 2021

Feel free to discard and close the underlying issue. Just wanted to confirm this works now and is intended to work in the future.

@eps1lon eps1lon marked this pull request as ready for review November 3, 2021 10:17
@gaearon gaearon closed this Nov 23, 2021
@eps1lon eps1lon deleted the test/opaque-identifier-two-way branch November 23, 2021 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Can't hydrate useOpaqueIdentifier generated object in another component
5 participants