Add fetchpriority to props #3093
Conversation
🦋 Changeset detectedLatest commit: 1714368 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
@DustinBrett Thanks for the PR. I was looking for this. |
No problem! I could add |
As you can see here, on @types/react it was renamed to camelCase format. |
Good points, thanks for the info. I've updated the PR. |
There was a problem hiding this comment.
Ah nice, I faced the same bug, landing here from https://performance.shopify.com/blogs/blog/optimizing-images-for-performance-on-shopify and mui/material-ui#38733.
| @@ -69,6 +69,8 @@ const props = { | |||
| draggable: true, | |||
| encType: true, | |||
| enterKeyHint: true, | |||
| fetchpriority: true, | |||
There was a problem hiding this comment.
Is this even valid?
| fetchpriority: true, |
There was a problem hiding this comment.
It depends what you mean by valid, but I would say likely yes. I use this and it does indeed function and have an effect in browsers. The links I provided in the description also use it this way.
Also checking the way this list is used, it seemed to me that both versions should be added as I saw nothing to turn the camel case version into lower case.
There was a problem hiding this comment.
I guess the handling of uppercase vs. lowercase confuses me a bit in @emotion/is-prop-valid.
References:
- Add
fetchPriorityto<img>and<link>facebook/react#25927 - Correct fetchPriority naming DefinitelyTyped/DefinitelyTyped#65972
Maybe we should actually .toLowerCase() when determining if a prop should be forwarded or not.
There was a problem hiding this comment.
Ya it's possible converting to lower case could be useful. I didn't see many duplicates. For my use case with Styled Components, it currently passes without the capitol. So I think it would be needed for this PR to stop my warning. The camel case version is not one I've had to use with React yet.
There was a problem hiding this comment.
I think we should stick to the camelCase here and follow React's suite.
There was a problem hiding this comment.
The issue is that the camelCase only solution is not supported with the latest Next.js w/React. Only in React 19 will they be moving to camelCase for this property. If I change to using camelCase on elements within my codebase I get an error, as do others which is mentioned in the links that @oliviertassinari posted a year ago.
|
This was mentioned as part of Safari 17.2 also btw. https://webkit.org/blog/14787/webkit-features-in-safari-17-2/#fetch-priority |
|
Can we get this moving forward? I would guess that the camel case variant is the one go with as metioned here: https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/fetchPriority |
DustinBrett
force-pushed
the
add-fetchpriority
branch
from
11d6b45 to
e923c0f
Compare
What:
Adding the now standardized
fetchpriorityproperty to the valid props list. "Fetch Priority" is now included in the HTML living standard. It's been in Chrome since 101-102 and is now coming to Safari 17.Why:
This is a useful property that I use throughout my site. In some places I am passing it to a styled component and because it uses emotions is-prop-valid check, I am getting a warning. I think this property will continue to grow in adoption and should be considered valid.
How:
Added the lowercased property name to
packages\is-prop-valid\src\props.js. Similar to what was done in the closed PR #2748.Checklist: