Make optional SVG favicon come after the fallback

[ciromattia]

Description

Make SVG favicon come after PNG one, because Chrome chooses the favicon which comes last, thus preferring PNG over SVG.

Documentation

See Chrome bugreport: https://bugs.chromium.org/p/chromium/issues/detail?id=1104663

Related Issues

Fixes #26083

[moonmeister]

Thanks! You'll probably need to update test snapshots to get tests to pass

[ciromattia]

Thanks! You'll probably need to update test snapshots to get tests to pass

uh, I edited directly from github, let me look at the whole contributing process :)

[moonmeister]

no worries if not, I'll have time in a couple days to finish it.

[trevorblades]

@moonmeister @ciromattia one possible reason for the tests not passing is mixed up brackets/parens in the code. I commented on the two lines in question, but here's a full example of what the implementation should be:

if (srcIconExists) {
  if (insertFaviconLinkTag) {
    favicons.forEach(favicon => {
      headComponents.push(
        <link
          key={`gatsby-plugin-manifest-icon-link-png`}
          rel="icon"
          href={withPrefix(
            addDigestToPath(favicon.src, cacheDigest, cacheBusting)
          )}
          type="image/png"
        />
      )
    })
    if (icon?.endsWith(`.svg`)) {
      headComponents.push(
        <link
          key={`gatsby-plugin-manifest-icon-link-svg`}
          rel="icon"
          href={withPrefix(
            addDigestToPath(`favicon.svg`, cacheDigest, cacheBusting)
          )}
          type="image/svg+xml"
        />
      )
    }
  }
}

[trevorblades]

This should just be a } since it's the end of a conditional statement

[trevorblades]

This should be }) since it's the end of a forEach loop

[trevorblades]

I've made these changes and PR'd them to @ciromattia's branch. They should be able to be merged in and hopefully tests will pass then 😄

We may still need to update snapshots since the order of <link>s changed, but I'll look into that now.

[moonmeister]

I've made these changes and PR'd them to @ciromattia's branch. They should be able to be merged in and hopefully tests will pass then 😄

We may still need to update snapshots since the order of <link>s changed, but I'll look into that now.

You're a gem. Thanks for doing this, I've been to busy for oss work.

[trevorblades]

No problem! I just ran tests and tried to update snapshots, but I guess none of them needed to be updated after all. I just opened #27843 (the branch I was PR'ing into this branch) to see if I could get the tests to pass there. I suppose if they do, merging that PR would be the same thing as merging my PR into this branch, and then this branch into gatsby/master.

[moonmeister]

Yeah, no need to make more work, if that passes I'll close this in favor of the other.

[moonmeister]

closing in favor of #27843