[gatsby-plugin-manifest] Generate a 32x32 favicon instead of generating from first manifest icon

[cycleseven]

Description

gatsby-plugin-manifest uses the first of the configured manifest icons to generate a favicon. This isn't necessarily intuitive, and means a 48x48 favicon is used by default, which is probably not the ideal size.

This PR decouples the favicon size from the configured manifest icons, and generates a 32x32 icon instead.

Some caveats

• This would technically introduce a breaking change: in "hybrid" mode, users can currently control the generated favicon size using the first element of icons. This change takes that control away.
• This PR introduces an edge case: a user could configure a path in their icons config that clashes with the fixed favicon path. As it stands, the favicon generation would overwrite the one from icons in that edge case. It's a slim chance this would have any impact imo: someone would have to want a non-32x32 icon called favicon-32x32.png, which would be a bit weird 😄
• A 32x32 PNG alone is probably still not the most widely compatible favicon setup. But it should be an improvement over 48x48. Users who want deeper control over favicons can still opt-out of gatsby-plugin-manifest's favicon generation as before.

See the related issue for more info and discussion.

Documentation

The README.md for gatsby-plugin-manifest is slightly tweaked to give more details about the generated favicon format.

Related Issues

Fixes #19899

[laurieontech]

This looks good from a docs perspective.

[moonmeister]

Looks good. That main function is sure getting hard to follow(not your fault). One day I'll get annoyed with it and clean up the code. The only thing I'm curious about is if there is a test for disabling the favicon, and confirming it doesn't get written..this should have already existed. Can you please check if it exists and and if not, please add that test (if you need help or just want me to do so just let me know). Thanks for the good work!

[cycleseven]

@moonmeister

The only thing I'm curious about is if there is a test for disabling the favicon, and confirming it doesn't get written..this should have already existed. Can you please check if it exists and and if not, please add that test (if you need help or just want me to do so just let me know)

Let me get back to you on this part.

There are already tests to ensure gatsby-ssr correctly includes/excludes the <link> here... but the plugin could use something similar on the gatsby-node side now, to show we don't generate an unnecessary PNG when include_favicon is false.

My first idea on how to test that was to assert that sharp is called the correct number of times with the expected parameters (ie. that a sharp call for the favicon is triggered by default, and doesn't happen when include_favicon is false).

But then I saw there was already at least one test that should cover the positive case, like this one. Which raises the question of why that didn't fail so far. In theory, I'd expect my code changes to modify the number of calls to sharp under that plugin config.

Since the content digest function is mocked to return a constant, and the icon generation is skipped if the content digest is already cached, then it actually seems like some existing tests will be asserting expect(sharp).toHaveBeenCalledTimes(n) for some n that doesn't match the real number of sharp calls outside the test environment 🤔 could be misunderstanding so far though.

Will try to take a closer look at this later in the week to figure out what's going on here and how best to add those tests 🤞

[moonmeister]

Really good work on tests and everything. Thanks!

[cycleseven]

Failing CI check, guess that change to the tests wasn't as safe as I thought...? 😞 can't see what the error in the Cloud Tests is from my end though. @moonmeister if you can let me know what exactly failed, I'm happy to finish this off from my end. Might be getting into poor feedback loop territory here though: also happy to hand this last part over to you if you're up for it?

[moonmeister]

That page isn't loading for me so not sure I can help. We may have to wait for a Gatsby Core team member to give us the details. Given everything passed on circle it might just be fluke. But, some hash algorithms are hardware dependent. Meaning it's possible whatever hardware is running in the cloud build is creating a different hash.

[cycleseven]

But, some hash algorithms are hardware dependent. Meaning it's possible whatever hardware is running in the cloud build is creating a different hash.

I guess I'd expect other tests that call the real createContentDigest to suffer from the same problem if that was it 🤔 but that does make sense in theory yeah, I guess there was a good reason to mock the function in the first place.

If that's the issue, it should work equally well to mock createContentDigest with an identity/stringify function instead.

jest.mock(`gatsby-core-utils`, () => {
  return {
    createContentDigest: jest.fn(x => `${x}`),
  }
})

Still outputs a unique value for each unique input, without depending on the real hash algo... so would tick the requirements for these tests, I think.

I'll wait on a Gatsby Core member to see what's up with that CI failure before moving forward though, might be counterproductive to try and fix this blind. Will keep my eye out for updates here 🤞 thanks for all the help so far.

[pvdz]

(Fwiw; The Cloud tests failure was unrelated to this PR, caused by npm noise in the CLI which triggered a non-zero exit by Gatsby. The noise is unrelated to the test. All tests passed.)

[moonmeister]

Awesome! well, then I think we're just waiting for @gatsbyjs/core team member to review and we should be good.

[pvdz]

Where do we find one of those?

[pvdz]

(Kidding. I'll make a superficial pass but I have zero insight into this plugin so somebody else will have to do the semantics)

[moonmeister]

(Kidding. I'll make a superficial pass but I have zero insight into this plugin so somebody else will have to do the semantics)

Well hopefully that was what my review was good for. I haven't actually tested the new plugin manually, but if tests are passing and @cycleseven has seen it work irl then we should be good.

[pvdz]

Ok, fair enough and well met :)

Code looks good. One question remaining for me is whether this is backwards compat breaking. Before it was taking the first image of the set. What happens to people that bump the package without changing their config?

[cycleseven]

@pvdz thanks for taking a look at this.

One question remaining for me is whether this is backwards compat breaking. Before it was taking the first image of the set. What happens to people that bump the package without changing their config?

So first it's worth making it clear the only possible breaking change here would be that the generated favicon resolution changes: when we say "first image of the set", that set configures the generated sizes, but it's from a single input image. Nobody can end up with the wrong favicon after this. Probably these changes will be completely unnoticeable for most eg. Chrome seems really good at handling different sizes identically.

But yeah as mentioned in the original description above, there's technically a breaking change for what I assume would be a small minority of users who might be taking advantage of an undocumented side effect of the config to generate a non-standard favicon size intentionally. To break this down:

Automatic mode

These users completely leave it up to the plugin to generate appropriate sizes for both manifest icons + favicon. For those users, this should just be a transparent browser compat improvement.

I assume this is the majority of users.

Manual mode

The plugin doesn't generate a favicon for these users, and it never did. No change here.

Hybrid mode

These users manually define the sizes they want for the manifest. Favicon is just an opt-in or opt-out, the docs make no promise about what size the favicon is or where that size comes from. But it's possible that some users a) have understood that the plugin uses the first manifest icon size for the favicon by experimenting or reading the source, b) intentionally want to control that size and c) want that size to be something other than 32x32.

From my (non-exhaustive) research, 32 really does look like the best supported size if you can only pick one, so it's hard to imagine this being a noticeable or bad change even for this group of users. But it's true that they will lose control that they technically had before, so that's a breaking change for sure.

I think we'd want to eventually make that breaking change in any case. But I'm also happy to submit a non-breaking version of this that only affects "automatic" mode to tide us over until a major version...? Up to you guys, let me know 👍

@moonmeister

@cycleseven has seen it work irl then we should be good

Yeah, I've tested this out with my real Gatsby project using gatsby-dev-cli, with both my original config, as well as just leaning on automatic mode. I assume this is enough? Give me a shout if you have something more in mind though.

[moonmeister]

Thanks for the details on the dev experience change. I'm comfortable pushing this as a bug fix. It doesn't require any config change and doesn't really add any features. If someone out there was hacking private implementation details rather than just opening an issue, is doubtful, and not supported.

That testing should be plenty.

[pvdz]

Thanks for the clarification :) And for the PR!

[gatsbot]

Holy buckets, @cycleseven — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

[moonmeister]

@cycleseven Thanks again for the work and the PR!

[cycleseven]

@moonmeister @laurieontech @pvdz thank you all for giving your time to help get this in 🎉

[pieh]

Published gatsby-plugin-manifest@2.3.6