Update preload links to support nonce and fetchpriority #26826
Conversation
|
Hi @liuyenwei! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
| as="script" | ||
| /> | ||
| </head> | ||
| <body>hello world</body> |
There was a problem hiding this comment.
I noticed the <script> elements were not appearing here - this seems weird to me, but looking through the other tests I'm assuming this is expected behavior?
There was a problem hiding this comment.
getMeaningfulChildren filters out some scripts like instruction scripts. Sometimes it can't tell whether a script is meaningful so you can render it with a specail attribute data-meaningful={true} and it will then be included if you really want to assert that the script is included. In this case though you are really just making sure the nonce and fetcpriority show up on the preload so I would leave it as is personally
| @@ -5510,6 +5510,7 @@ function preloadAsStylePropsFromProps(href: string, props: any): PreloadProps { | |||
| as: 'style', | |||
| href: href, | |||
| crossOrigin: props.crossOrigin, | |||
| fetchPriority: props.fetchPriority, | |||
There was a problem hiding this comment.
I didn't make these changes to preloadPropsFromPreloadOptions. It seems like that's used for ReactDOM.preload - please let me know if I should make similar changes there as well
There was a problem hiding this comment.
@liuyenwei we should support this for preload and preinit functions if you want to open a separate PR for that
There was a problem hiding this comment.
sounds good, i can make that change as well. thanks @gnoff !
|
Comparing: f8de255...957b673 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) |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
@liuyenwei thank you for putting this together. The PR is good and we could merge. However in thinking through our strategy on automatic preloads we're going to actually unwind this feature altogether. For Hoistable Elements and Resources we will do intelligent re-ordering and (for resources) deduplication and preloading. But doing preloads for sync scripts and non-precedence stylesheets doesn't make a lot of sense
for scripts, as much as possible you should be using async. If you need sync or defer they should probably go in the preloads of your bootstrap. For other use-cases you can manually preload them using ReactDOM.preload() or just by rendering a preload tag.
For stylesheets, you are almost always going to need to render those in the <head> anyway where they will block paint so the preload doesn't help and actually hurts because it adds unnecessary bytes. You can't really put a stylehseet arbitrarily your tree b/c of precedence issues. So the use-case for preloading is really weak there. And again if you desire to have them preloaded there is always the explicit methods by which to do so.
I'll merge this for now b/c we may publish a canary before I remove the preload behavior, but just understand that it will be removed shortly.
The plan for the followup update is to remove these auto preloads and expose fetchPriority as an option for ReactDOM.preload() and ReactDOM.preinit().
| as="script" | ||
| /> | ||
| </head> | ||
| <body>hello world</body> |
There was a problem hiding this comment.
getMeaningfulChildren filters out some scripts like instruction scripts. Sometimes it can't tell whether a script is meaningful so you can render it with a specail attribute data-meaningful={true} and it will then be included if you really want to assert that the script is included. In this case though you are really just making sure the nonce and fetcpriority show up on the preload so I would leave it as is personally
Currently when React generates rel=preload link tags for script/stylesheet resources, it will not carryover nonce and fetchpriority values if specified on the original elements. This change ensures that the preload links use the nonce and fetchPriority values if they were specified. DiffTrain build for [535c038](535c038)
Currently when React generates rel=preload link tags for script/stylesheet resources, it will not carryover nonce and fetchpriority values if specified on the original elements. This change ensures that the preload links use the nonce and fetchPriority values if they were specified.
Currently when React generates rel=preload link tags for script/stylesheet resources, it will not carryover nonce and fetchpriority values if specified on the original elements. This change ensures that the preload links use the nonce and fetchPriority values if they were specified. DiffTrain build for commit 535c038.
Summary
Addresses #26810 and #26781
Currently when React generates
rel=preloadlink tags for script/stylesheet resources, it will not carryovernonceandfetchpriorityvalues if specified on the original elements.This change ensures that the preload links use the
nonceandfetchPriorityvalues if they were specified.How did you test this change?
Tested the change by building a local copy and using it in my application.
Before the change:
After the change, I can confirm that:
fetchpriority(for both script and style preloads) andnonce(for script preloads)