Skip to content

Float: Stringify crossorigin="anonymous" #28958

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
wants to merge 4 commits into from

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Apr 30, 2024

Summary

<img crossOrigin="anonymous" /> now creates the corresponding <link rel="preload" /> tag preserving crossOrigin since anonymous is a valid value for crossOrigin.

It doesn't change production behavior since it results in the same state. Mainly to just reduce confusion but then crossOrigin="some-non-existing-value" would also result in crossorigin="" so maybe that's fine?

How did you test this change?

Added test for <img crossOrigin="anonymous" />. Other tests were also updated so I'm curious if ignoring anonymous is intended for preconnect.

I also added a test showing how crossOrigin doesn't factor into keying. Mostly to show the difference between how Float and next/legacy/image differs with regard to preloading. next/legacy/image currently adds a <link rel="preload" /> matching crossOrigin of the last <img /> with the same key whereas Float uses crossOrigin from the first image.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 30, 2024
@react-sizebot
Copy link

react-sizebot commented Apr 30, 2024

Comparing: d779eba...d5bdae7

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.js +0.78% 6.66 kB 6.71 kB +0.77% 1.82 kB 1.84 kB
oss-experimental/react-dom/cjs/react-dom.production.js +0.78% 6.67 kB 6.72 kB +0.77% 1.83 kB 1.84 kB
facebook-www/ReactDOM-prod.classic.js = 591.11 kB 591.16 kB +0.01% 103.94 kB 103.95 kB
facebook-www/ReactDOM-prod.modern.js = 567.33 kB 567.39 kB +0.02% 100.34 kB 100.36 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-dom/cjs/react-dom.react-server.production.js +1.07% 4.84 kB 4.89 kB +1.29% 1.24 kB 1.26 kB
oss-stable-semver/react-dom/cjs/react-dom.react-server.production.js +1.07% 4.84 kB 4.89 kB +1.29% 1.24 kB 1.26 kB
oss-stable/react-dom/cjs/react-dom.react-server.production.js +1.07% 4.84 kB 4.89 kB +1.29% 1.24 kB 1.26 kB
oss-stable-semver/react-dom/cjs/react-dom.production.js +0.78% 6.64 kB 6.69 kB +0.83% 1.80 kB 1.81 kB
oss-stable/react-dom/cjs/react-dom.production.js +0.78% 6.66 kB 6.71 kB +0.77% 1.82 kB 1.84 kB
oss-experimental/react-dom/cjs/react-dom.production.js +0.78% 6.67 kB 6.72 kB +0.77% 1.83 kB 1.84 kB
oss-experimental/react-dom/cjs/react-dom.react-server.development.js +0.30% 16.55 kB 16.60 kB +0.30% 3.61 kB 3.62 kB
oss-stable-semver/react-dom/cjs/react-dom.react-server.development.js +0.30% 16.55 kB 16.60 kB +0.30% 3.61 kB 3.62 kB
oss-stable/react-dom/cjs/react-dom.react-server.development.js +0.30% 16.55 kB 16.60 kB +0.30% 3.61 kB 3.62 kB
oss-stable-semver/react-dom/cjs/react-dom.development.js +0.20% 24.46 kB 24.51 kB +0.18% 6.23 kB 6.24 kB
oss-stable/react-dom/cjs/react-dom.development.js +0.20% 24.49 kB 24.54 kB +0.19% 6.26 kB 6.27 kB
oss-experimental/react-dom/cjs/react-dom.development.js +0.20% 24.50 kB 24.55 kB +0.19% 6.26 kB 6.27 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against d5bdae7

@eps1lon eps1lon force-pushed the float-crossorigin branch 2 times, most recently from d172865 to 070ed36 Compare April 30, 2024 14:55
@eps1lon eps1lon force-pushed the float-crossorigin branch from 070ed36 to 82d10cf Compare April 30, 2024 15:05
@eps1lon eps1lon marked this pull request as ready for review April 30, 2024 15:10
@eps1lon eps1lon requested a review from gnoff April 30, 2024 15:10
@eps1lon eps1lon changed the title Float: Add support for crossOrigin="anonymous" Float: Stringify crossorigin="anonymous" Apr 30, 2024
@gnoff
Copy link
Collaborator

gnoff commented May 2, 2024

The spec doesn't show it clearly but the invalid value state is also anonymous so conceptually I've been thinking about crossorigin as just a tri-value. missing, "", and "use-credentials" to map to no cors, anon cors, and credentials

The other reason to use empty string aggressively is that it's just shorter. Are you concerned that folks will look at the tag and think something is wrong? or that the spec might change in the future and make this optimization semantically breaking? (I doubt they could do the latter given the hesitance to break the web so I feel pretty confident we can leave this be)

@eps1lon
Copy link
Collaborator Author

eps1lon commented May 2, 2024

The other reason to use empty string aggressively is that it's just shorter. Are you concerned that folks will look at the tag and think something is wrong?

I did 😅 Mainly because I got confused by the spec (empty value and crossorigin meaning the same thing) but also because it differs from how we SSR <img crossOrigin="whatever" />.

I wouldn't have filed this if I understood that the empty value is the same as crossorigin="anonymous". But now that it's open, and in the light of how difficult it is to land new attribute (values) in React, I'm wondering if we should optimize for bytes saved or future compatibility (i.e. don't normalize the string value at all and stringify it as-is). In the end, people can still opt for bytes saved by passing crossOrigin: '', no?

@eps1lon eps1lon force-pushed the float-crossorigin branch from 5c62fa6 to d5bdae7 Compare May 2, 2024 21:16
@gnoff
Copy link
Collaborator

gnoff commented May 2, 2024

Well, the thing that is different here is you aren't rendering the preload link yourself. This tag is owned by the react runtime so we're not really altering a user value. I suppose you could argue that ReactDOM.preload(..., { crossOrigin: "anonymous" }) is a user value but even still I think you are saying you want anonymous CORS mode not that you want a specific tag with a specific attribute.

So I don't think we need to extrapolate too much that we're different if you render you get one serialization but the preload link doesn't match

@eps1lon
Copy link
Collaborator Author

eps1lon commented May 3, 2024

Yeah makes sense

@eps1lon eps1lon closed this May 3, 2024
@eps1lon eps1lon deleted the float-crossorigin branch May 3, 2024 08:16
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