fix(react): dedupe prefetch links #7060
Conversation
markdalgleish
added
bug
🦋 Changeset detectedLatest commit: ee1a692 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
| @@ -360,7 +360,7 @@ export function Links() { | |||
|
|
|||
| return ( | |||
| <link | |||
| key={link.rel + (link.href || "") + (imageSrcSet || "")} | |||
| key={key} | |||
There was a problem hiding this comment.
They key is now the exact same string used for deduping. To enable this, the utilities that return arrays of link descriptors now provide Keyed objects that expose a key property for each link.
This ensures we never get any duplicate key errors, as opposed to the previous logic which could result in duplicate keys if rel, href and/or imageSrcSet properties were the same but other properties differed.
| ? ({ ...link, rel: "prefetch" } as HtmlLinkDescriptor) | ||
| : ({ ...link, rel: "prefetch", as: "style" } as HtmlLinkDescriptor) | ||
| ); | ||
| return dedupeLinkDescriptors( |
There was a problem hiding this comment.
This is the core fix for #5677. The prefetch links weren't deduped as they were in getLinksForMatches (now called getKeyedLinksForMatches).
| if (!set.has(str)) { | ||
| set.add(str); | ||
| deduped.push(descriptor); | ||
| let key = JSON.stringify(sortKeys(descriptor)); |
There was a problem hiding this comment.
This fixes an unreported bug where the deduping didn't work correctly if properties were specified in a different order. This behaviour is covered in the newly added test.
| let set = new Set(); | ||
| let preloadsSet = new Set(preloads); | ||
|
|
||
| return descriptors.reduce((deduped, descriptor) => { | ||
| let alreadyModulePreload = | ||
| preloads && |
There was a problem hiding this comment.
Since the preloads array is now optional, we bail out early if it hasn't been provided.
| .filter((link) => link.rel === "stylesheet" || link.rel === "preload") | ||
| .map((link) => | ||
| link.rel === "stylesheet" | ||
| ? ({ ...link, rel: "prefetch", as: "style" } as HtmlLinkDescriptor) |
There was a problem hiding this comment.
This was just a slight refactor for legibility since link.rel === "stylesheet" was checked first in the earlier filter.
| // these don't spread `linkProps` because they are full link descriptors | ||
| // already with their own props | ||
| <link key={link.href} {...link} /> | ||
| <link key={key} {...link} /> |
There was a problem hiding this comment.
This fixes an unreported bug where responsive image prefetch links would all have a key of undefined since they have imgSrcSet and imgSizes properties but no href property.
| export async function getStylesheetPrefetchLinks( | ||
| export type KeyedHtmlLinkDescriptor = { key: string; link: HtmlLinkDescriptor }; | ||
|
|
||
| export async function getKeyedPrefetchLinks( |
There was a problem hiding this comment.
I renamed this from getStylesheetPrefetchLinks to getKeyedPrefetchLinks because, apart from now being an array of objects with key and link properties so consumers don't need to generate their own keys, it also makes it clearer that this code also converts preload links into prefetches (as seen on line 330).
github-actions
bot
added
the
awaiting release
|
🤖 Hello there, We just published version Thanks! |
|
🤖 Hello there, We just published version Thanks! |
Closes #5677
Fixing the linked issue highlighted a number of bugs in the code related to deduping links and generating keys for rendered links. I've added comments below to highlight the notable changes.