-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-plugin-manifest): Update manifest on navigation #17426
Conversation
20d343e
to
7e77d8f
Compare
Hm, not sure why the e2e test is failing since I use the asset prefix when setting the manifest: https://github.com/gatsbyjs/gatsby/pull/17426/files#diff-9d857a76b97041cf4aefbf1950cd3c9fR11 |
Hoping I can also get some help regarding this ESLint warning I get when working on the plugin as a local plugin:
Babel adds "use strict" to the top of every file and ESLint (I'm using Gatsby's default config) complains about it. Have you encountered this before when working on plugins as a local plugin? Update: I filed a separate issue with a test repo to demonstrate the problem: #17489 |
I'm pretty confident that this is ready to be shipped.
I think we can iterate on this if the startWith/regex doesn't suffice. Maybe you have some final comments @moonmeister |
@moonmeister, yeah offline plugin has issues with it. I think it's good to just keep the default one manifest.webmanifest and only suffix the ones that are in the localized string. Thanks for pointing out gatsby-ssr, I forgot about it and looks like I didn't have any issue because of rehydration XD. The latest offline plugin doesn't prefetch manifest anymore when it's not found so the issue looks like it doesn't exist anymore. I'll bring that feature back as it's a regression. I updated tests and I think, it's ready to go. |
What about caching the non-default manifests? |
They will be cached when navigating. I'll fix the regression of manifest prefetching on first load in gatsby-plugin-offline but it's not a problem of this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ship this. Thanks @dkthehuman & @moonmeister
Description
With the addition of the
localize
option ingatsby-plugin-manifest
(#13471), multiple manifests can be generated and served depending on the path of the URL. While the correct manifest is served on initial page load, the link to the manifest is never updated even if the user navigates to a page that should include a different manifest.This PR hooks into
onRouteChange
ingatsby-browser.js
in order to update the manifest as necessary.Related Issues
Fixes #17255