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

[React 19] React warns about "fetchpriority" props #28946

Open
ascorbic opened this issue Apr 29, 2024 · 10 comments
Open

[React 19] React warns about "fetchpriority" props #28946

ascorbic opened this issue Apr 29, 2024 · 10 comments

Comments

@ascorbic
Copy link
Contributor

ascorbic commented Apr 29, 2024

Summary

In 18.2.0, React allowed the fetchpriority prop in all lower case, but did not accept camelized fetchPriority. Support for the camelized version was added in #25927, but it seems to have broken the lowercased version. This means there is no variant that works in both 18.2 and 18.3 canary/19 beta.

Repro: https://codesandbox.io/p/sandbox/nameless-bird-wvv84h

In 18.2.0 fetchPriority generates a warning but fetchpriority works without warning
In 18.3.0-canary and later, fetchPriority works but fetchpriority generates a warning.

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 29, 2024

In the repro, the fetchpriority attribute is set for both cases. The warning targets different casings now. Is there some other behavior that changed?

@ascorbic
Copy link
Contributor Author

Both should work correctly, with no warnings. fetchpriority works in 18.2.0. Adding support for fetchPriority shouldn't have broken the previously-supported casing, but seems to have done so.

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 29, 2024

With broken you mean the warning that changed or is there some other, end-user observable behavior that broke?

@ascorbic
Copy link
Contributor Author

I mean the warning. It warns when previously it didn't complain. I think this would count as a breaking change. It renders fine in both cases, so the warning isn't needed.

@rickhanlonii
Copy link
Member

rickhanlonii commented Apr 30, 2024

It doesn't count as a breaking change, since we don't consider DEV warnings as breaking changes in our development policy: https://react.dev/community/versioning-policy#what-counts-as-a-breaking-change

But we should probably make this a deprecation warning instead of the current text since it implies it doesn't work. Something like:

Warning In React 19,fetchpriority has been renamed to fetchPriority. We will remove support fetchpriority in a future version. Please check the render method of <Foo>.

What do you think @eps1lon?

@ascorbic
Copy link
Contributor Author

Are you planning to remove support? I'm not clear on the policy there: is the plan to only support DOM properties and not HTML attributes?

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 30, 2024

Mabye the "invalid" wording is a bit strong. We should just say it's renamed as @rickhanlonii suggests. But removing support would be a novel thing to do. We haven't done that for any other prop.

@ascorbic
Copy link
Contributor Author

ascorbic commented Apr 30, 2024

Is there any reason to not treat them as extra variants that are both supported (like Preact does), and not warn about either? One is the HTML attribute and the other is the DOM property and they both work.

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 30, 2024

It can get confusing with regards to object spreading and handling both casings. We'd have to add additional runtime logic to reconcile all casing variants.

Typechecking would also have to support both casings and would no longer flag if you specify both properties.

@ascorbic
Copy link
Contributor Author

ascorbic commented May 2, 2024

I wish there was a way to feature-detect for it though. A warning is fine for end users, but those of us building libraries will need to support both. Granted, there probably aren't many libraries that need to set that particular prop (and with vercel/next.js#65235 and ascorbic/unpic-img#644 those are probably most covered), but checking for the existence of use isn't a great solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants