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 media option to ReactDOM.preload() #27129

Closed

Conversation

damnsamn
Copy link

Summary

Similar to #27096, we've encountered an issue in Next.js while using ReactDOM.preload(), where adding a media attribute does nothing.

I've not contributed to OSS before and am not familiar with the React core codebase, so I've followed the example set by the author of the #27096 pull request as a basis for this change.

How did you test this change?

I wrote the test within ReactDOMFloat-test.js before making any functional changes, and ran:

yarn test packages/react-dom/src/__tests__/ReactDOMFloat-test.js

both before and after making the functional changes to verify that the test failed before the change, and passed after.

@react-sizebot
Copy link

react-sizebot commented Jul 19, 2023

Comparing: 72411c4...ef3bac4

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 = 176.24 kB 176.24 kB = 54.88 kB 54.88 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 178.34 kB 178.34 kB = 55.53 kB 55.53 kB
facebook-www/ReactDOM-prod.classic.js = 571.94 kB 571.94 kB = 100.56 kB 100.56 kB
facebook-www/ReactDOM-prod.modern.js = 555.70 kB 555.70 kB = 97.62 kB 97.62 kB
test_utils/ReactAllWarnings.js Deleted 67.55 kB 0.00 kB Deleted 16.54 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
test_utils/ReactAllWarnings.js Deleted 67.55 kB 0.00 kB Deleted 16.54 kB 0.00 kB

Generated by 🚫 dangerJS against ef3bac4

@vankatadam
Copy link

vankatadam commented Jan 24, 2024

Any progress on this? This feature would be really needed, especially for NextJS users as they disabled custom Head in the AppDir. As a solution to some missing features for preloading they just refer to ReactDOM.preload(): https://nextjs.org/docs/app/api-reference/functions/generate-metadata#resource-hints

@eps1lon eps1lon requested a review from gnoff January 24, 2024 16:31
@eps1lon
Copy link
Collaborator

eps1lon commented Jan 24, 2024

@damnsamn I believe support was added in #27641 but the additional test would still be welcome. Can you rebase and resolve conflicts?

@damnsamn damnsamn force-pushed the add-media-on-reactdom-preload branch from 11a175f to ef3bac4 Compare January 25, 2024 02:44
@damnsamn
Copy link
Author

damnsamn commented Jan 25, 2024

@eps1lon I've rebased, but the changes in packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js and packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js were to functions that don't exist anymore. I don't really have a sound enough understanding of what's changed and where I should be re-making this change to resolve the conflicts.

I've retained the test and type updates though, and the test is failing - so #27641 probably hasn't solved this issue as far as I can tell.

Copy link

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 24, 2024
@eps1lon eps1lon removed the Resolution: Stale Automatically closed due to inactivity label Apr 24, 2024
@eps1lon
Copy link
Collaborator

eps1lon commented May 1, 2024

Fixed in #28635 where tests were also added. Sorry for not getting around to this earlier.

@eps1lon eps1lon closed this May 1, 2024
@damnsamn
Copy link
Author

damnsamn commented May 2, 2024

Appreciate that it happened at all, thanks mate!

@damnsamn damnsamn deleted the add-media-on-reactdom-preload branch May 2, 2024 00:44
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.

5 participants