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

fix: Better support for imageSrcSet + imageSizes in route links (#184) #2944

Merged
merged 8 commits into from
May 20, 2022

Conversation

chaance
Copy link
Collaborator

@chaance chaance commented Apr 20, 2022

This PR improves support for returned links that include an imagesrcset attribute to preload images when matched against a responsive set of URLs. This only works when rel === 'preload' and as === 'image'. https://html.spec.whatwg.org/commit-snapshots/cb4f5ff75de5f4cbd7013c4abad02f21c77d4d1c/#attr-link-imagesrcset

Prior to this PR, a few problems with our implementation:

  1. Our types for HtmlLinkDescriptor make href required. This is true unless a link includes imagesrcset and imagesizes, though imagesrcset and href will also work with or without imagesizes according to the spec. This PR uses an intersection type to account for these cases.
  2. React 17 was released prior to these attributes being accepted into the spec and implemented by browsers, so passing the props as camel-cased will throw a warning about unsupported DOM props. Our types recognized this and supported the all-lowercase version to skip the warning and forward the attribute to the node, but React 18 now recognizes the camel-cased version. This PR updates our types to use the camel-cased prop names, but it will support either at runtime and normalize the object based on the detected version of React.
  3. Because we use href + rel as a key, links without an href will have duplicate keys. So imageSrcSet is added as a fallback to prevent that.

Closes: #184

  • Docs
  • Tests

@chaance chaance changed the title fix: support for imageSrcSet + imageSizes in links (#184) fix: Better support for imageSrcSet + imageSizes in route links (#184) Apr 20, 2022
@ryanflorence
Copy link
Member

I’ve found these kinds of tests to be flakey because the browser can decide not to prefetch given different memory/cpu/who-knows-what. Maybe we can assert on the rendered link tags instead of requests?

@ryanflorence
Copy link
Member

Also, maybe we can just use the DOM types here?

@chaance
Copy link
Collaborator Author

chaance commented Apr 20, 2022

Also, maybe we can just use the DOM types here?

Unfortunately the DOM types also require href so this wouldn't really fix things for our end users, and React's LinkHTMLAttributes type is different for 17 vs. 18. I think our custom types are a big DX improvement for both, personally.

@chaance chaance requested a review from MichaelDeBoey May 19, 2022 17:34
@chaance chaance merged commit d19d888 into dev May 20, 2022
@chaance chaance deleted the chance/optional-href branch May 20, 2022 13:34
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-d19d888-20220521 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-d19d888-20220522 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

justjavac pushed a commit to justjavac/remix that referenced this pull request Jun 11, 2022
…s` (remix-run#184) (remix-run#2944)

Improves support for returned links that include an `imagesrcset` attribute to
preload images when matched against a responsive set of URLs. This only works
when `rel === 'preload'` and `as === 'image'`.
https://html.spec.whatwg.org/commit-snapshots/cb4f5ff75de5f4cbd7013c4abad02f21c77d4d1c/#attr-link-imagesrcset

Prior to this change, a few problems with our implementation:

1. Our types for `HtmlLinkDescriptor` make `href` required. This is true
   _unless_ a link includes `imagesrcset` and `imagesizes`, though `imagesrcset`
   and `href` will also work with or without `imagesizes` according to the spec.
   This change uses an intersection type to account for these cases.
2. React 17 was released prior to these attributes being accepted into the spec
   and implemented by browsers, so passing the props as camel-cased will issue a
   warning about unsupported DOM props. Our types recognized this and supported
   the all-lowercase version to skip the warning and forward the attribute to
   the node, but React 18 now recognizes the camel-cased version. This change
   updates our types to use the camel-cased prop names, but it will support
   either at runtime and normalize the object based on the detected version of
   React.
3. Because we use `href` + `rel` as a key, links without an `href` will have
   duplicate keys. So `imageSrcSet` is added as a fallback to prevent that.
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v1.6.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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.

4 participants