-
Notifications
You must be signed in to change notification settings - Fork 50.5k
[flags] make enableTrustedTypesIntegration dynamic
#35646
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
base: main
Are you sure you want to change the base?
Conversation
Sets the dynamic fork to VARIANT to ensure all tests exercise that code path and enables the integration in experimental releases.
rickhanlonii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To roll this out I need to test it at meta, so we're not ready to enable this in experimental yet.
|
Would it be worth to land the VARIANT change without the experimental upgrade? If so, happy to split that out. |
packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js
Outdated
Show resolved
Hide resolved
|
@hybrist yeah, i pushed to this branch to switch to We can merge this and I'll test it. If all goes well, this can probably go to experimental soonish, and could land in a minor. |
enableTrustedTypesIntegration dynamic
|
Thanks for the help getting this into shape! Sorry, could've caught the flag testing pattern myself. :) |
|
Comparing: 8c34556...a84f095 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
Set the dynamic fork to VARIANT to ensure all tests exercise the
enableTrustedTypesIntegrationcode path and enable the integration (e.g. dropping the forced stringification) in experimental releases.Historically, the main issue was an old jsdom version and IE9 from what I can tell.
Summary
It looks like this was kind of meant to become a default while back (see #27859) and I don't see any clear signal on why it didn't. So this PR is asking the question... maybe it should just happen now?
How did you test this change?
Existing tests are passing with this change. It looks like this also might have already been tested internally at Facebook (?).