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

Add support for 'crossorigin' attribute on bootstrapScripts and bootstrapModules #26844

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

HenriqueLimas
Copy link
Contributor

Summary

When using window.onerror in the bootstrap script that is hosted in a CDN the error message will show "Script error." instead of a proper error message. This is because of the cross-origin script.
Use crossOrigin naming convention, following the same name used in other configurations like preinit and preconnect

Fixes #25989

How did you test this change?

Used yarn test and added tests for the new config making sure that script tag has the crossorigin attribute

@react-sizebot
Copy link

Comparing: 535c038...bc25880

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 = 164.23 kB 164.23 kB = 51.77 kB 51.77 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 171.59 kB 171.59 kB = 53.98 kB 53.98 kB
facebook-www/ReactDOM-prod.classic.js = 570.55 kB 570.55 kB = 100.66 kB 100.66 kB
facebook-www/ReactDOM-prod.modern.js = 554.29 kB 554.29 kB = 97.84 kB 97.84 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOMServerStreaming-prod.modern.js +0.42% 141.46 kB 142.05 kB +0.21% 26.81 kB 26.86 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.production.min.js +0.26% 60.50 kB 60.65 kB +0.25% 18.67 kB 18.72 kB
oss-stable/react-dom/umd/react-dom-server.browser.production.min.js +0.26% 60.52 kB 60.68 kB +0.27% 18.69 kB 18.74 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.production.min.js +0.26% 60.34 kB 60.49 kB +0.20% 18.42 kB 18.45 kB
oss-stable/react-dom/cjs/react-dom-server.browser.production.min.js +0.26% 60.36 kB 60.52 kB +0.20% 18.44 kB 18.48 kB
oss-experimental/react-dom/cjs/react-dom-static.browser.production.min.js +0.25% 62.22 kB 62.37 kB +0.09% 19.27 kB 19.28 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js +0.25% 62.34 kB 62.50 kB +0.07% 19.32 kB 19.33 kB
oss-stable-semver/react-dom/cjs/react-dom-server.bun.production.min.js +0.25% 62.76 kB 62.91 kB +0.12% 18.85 kB 18.87 kB
oss-stable/react-dom/cjs/react-dom-server.bun.production.min.js +0.25% 62.78 kB 62.94 kB +0.13% 18.88 kB 18.90 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js +0.25% 62.50 kB 62.65 kB +0.09% 19.57 kB 19.59 kB
oss-experimental/react-dom/cjs/react-dom-static.edge.production.min.js +0.25% 62.56 kB 62.71 kB +0.06% 19.39 kB 19.40 kB
oss-stable-semver/react-dom/cjs/react-dom-server.edge.production.min.js +0.24% 64.58 kB 64.73 kB +0.17% 19.84 kB 19.87 kB
oss-stable/react-dom/cjs/react-dom-server.edge.production.min.js +0.24% 64.61 kB 64.76 kB +0.17% 19.86 kB 19.90 kB
oss-experimental/react-dom/cjs/react-dom-server.bun.production.min.js +0.24% 65.41 kB 65.56 kB +0.14% 20.03 kB 20.06 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.production.min.js +0.23% 66.69 kB 66.85 kB +0.17% 20.81 kB 20.84 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.production.min.js +0.23% 64.72 kB 64.87 kB +0.19% 19.90 kB 19.93 kB
oss-stable/react-dom/cjs/react-dom-server.node.production.min.js +0.23% 64.75 kB 64.90 kB +0.19% 19.92 kB 19.96 kB
oss-experimental/react-dom/cjs/react-dom-static.node.production.min.js +0.22% 66.79 kB 66.94 kB +0.20% 20.86 kB 20.90 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +0.22% 66.83 kB 66.98 kB +0.23% 20.88 kB 20.92 kB

Generated by 🚫 dangerJS against bc25880

Comment on lines 270 to 271
const crossOrigin =
typeof scriptConfig === 'string' ? undefined : scriptConfig.crossOrigin;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For crossorigin we have normalized this to "use-credentials" | "" | null because unless you are using use-credentials all string values behave identically to "anonymous" and "" is the shortest bytes-wise.

@@ -282,6 +286,12 @@ export function createResponseState(
stringToChunk(escapeTextForBrowser(integrity)),
);
}
if (crossOrigin) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would fail for crossOrigin: "". Should be something like if (typeof crossOrigin === 'string')

@HenriqueLimas HenriqueLimas requested a review from gnoff June 1, 2023 03:38
* shortest bytes-wise.
*/
function getCrossOrigin(options?: mixed) {
return options == null || typeof options.crossOrigin !== 'string'
Copy link

Choose a reason for hiding this comment

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

I think overall nested ternary expressions require more mental overhead to reason about or to edit in the future..

if (options == null || typeof options.crossOrigin !== 'string') {
  return null;
}
if (options.crossOrigin === 'use-credentials') {
  return 'use-credentials';
}
return '';

Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to follow up. I think this is close, feel free to @ me If I don't respond within a couple days of your next updates.

Thanks for submitting this PR, I really appreciate it :)

* Unless using "use-credentials" all the other strings behave like "anonymous" and "" is
* shortest bytes-wise.
*/
function getCrossOrigin(options?: mixed) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd revert the commit where you extract this function and leave it inline.

It works today that the script desciptors and the preconnect options happen to correlate and so this function works but it's essentially duck-typing a mixed object that provides no guarantees it will continue to stay in the shape it currently is in.

We'd hope closure would also inline these functions anyway and the logic isn't particularly complicated so we should just leave them inline

A compromise might be to only extract the function

function getCrossOriginFromString(crossOriginString: string): string {
  return crossOriginString === 'use-credentials' ? crossOriginString : ""
}

and then just use it once you've checked that the cross origin config is known to not be null. closure will almost certainly inline this and it does not presume any particular shape of options/configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Let me do this.

@HenriqueLimas
Copy link
Contributor Author

@gnoff reverted the commit. The ci/circleci: download_base_build_for_sizebot is failing, but as far is my understanding was a network issue, is it possible to re-trigger it?

@gnoff gnoff merged commit 90229eb into facebook:main Jun 13, 2023
github-actions bot pushed a commit that referenced this pull request Jun 13, 2023
…trapModules (#26844)

base build ci job failing but this change is unrelated and I think it is just flake with the builds host application

DiffTrain build for [90229eb](90229eb)
@HenriqueLimas HenriqueLimas deleted the add-crossorigin-support branch June 13, 2023 17:11
gnoff added a commit that referenced this pull request Jun 13, 2023
…s and bootstrap modules (#26942)

The recently merged support for crossorigin in bootstrap scripts did not
implement the functionality for preloading. This adds it

see #26844
github-actions bot pushed a commit that referenced this pull request Jun 13, 2023
…s and bootstrap modules (#26942)

The recently merged support for crossorigin in bootstrap scripts did not
implement the functionality for preloading. This adds it

see #26844

DiffTrain build for [a7bf5ba](a7bf5ba)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…trapModules (facebook#26844)

base build ci job failing but this change is unrelated and I think it is just flake with the builds host application
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…s and bootstrap modules (facebook#26942)

The recently merged support for crossorigin in bootstrap scripts did not
implement the functionality for preloading. This adds it

see facebook#26844
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…trapModules (#26844)

base build ci job failing but this change is unrelated and I think it is just flake with the builds host application

DiffTrain build for commit 90229eb.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…s and bootstrap modules (#26942)

The recently merged support for crossorigin in bootstrap scripts did not
implement the functionality for preloading. This adds it

see #26844

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

Feat(renderToPipeableStream): Allow passing crossorigin attribute on bootstrapScripts
5 participants