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

feat(gatsby-plugin-manifest): Update manifest on navigation #17426

Merged
merged 6 commits into from Sep 26, 2019

Conversation

@dkthehuman
Copy link
Contributor

dkthehuman commented Sep 5, 2019

Description

With the addition of the localize option in gatsby-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 in gatsby-browser.js in order to update the manifest as necessary.

Related Issues

Fixes #17255

@dkthehuman dkthehuman requested a review from gatsbyjs/core as a code owner Sep 5, 2019
@dkthehuman dkthehuman requested a review from moonmeister Sep 5, 2019
@dkthehuman

This comment has been minimized.

Copy link
Contributor Author

dkthehuman commented Sep 5, 2019

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

@lannonbr lannonbr changed the title Update manifest on navigation feat(gatsby-plugin-manifest): Update manifest on navigation Sep 7, 2019
@dkthehuman

This comment has been minimized.

Copy link
Contributor Author

dkthehuman commented Sep 8, 2019

Hoping I can also get some help regarding this ESLint warning I get when working on the plugin as a local plugin:

Module Warning (from ./node_modules/eslint-loader/index.js):

/Users/dskang/Code/dkthehuman.com/plugins/gatsby-plugin-manifest-multiple/gatsby-browser.js
  1:1  warning  'use strict' is unnecessary inside of modules  strict

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

@wardpeet

This comment has been minimized.

Copy link
Member

wardpeet commented Sep 24, 2019

I'm pretty confident that this is ready to be shipped.

  • fixed default localized manifest & sw support (gatsby-plugin-offline wasn't caching the correct file)
  • Only add gatsby-browser code when necessary

I think we can iterate on this if the startWith/regex doesn't suffice.

Maybe you have some final comments @moonmeister

@moonmeister

This comment has been minimized.

Copy link
Contributor

moonmeister commented on packages/gatsby-plugin-manifest/src/gatsby-node.js in 626efed Sep 24, 2019

Gatsby-ssr doesn't include a shouldLocalize option when calculating a suffix for the manifest link tag. Won't this break that? Also, I'm not entirely understanding why we can't add this sufffix to the default manifest here, can you explain?

This comment has been minimized.

Copy link
Contributor

moonmeister replied Sep 25, 2019

@wardpeet Yeah, so did some closer inspection on the manifest and offline plugins.

First, there was no particular reason we added the lang to the default manifest other than it was easier to keep it consistent than make an exception. We definitely will need to fix the SSR though, and it won't be as easy to detect the 'default' case. That said if this is the easiest fix that's fine with me...I'm not convinced it is...

If offline needs to cache a manifest, doesn't it need to cache all the manifests? if I load the es locale won't my manifest be missing from the offline cache if we're hard coding it to the default manifest?

if we modified : https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-offline/src/gatsby-node.js#L104

to be something like this:

  const manifests = [`manifest.json`, `**.webmanifest`]
  manifests.forEach(file => {
    if (glob.existsSync(`${rootDir}/${file}`)) globPatterns.push(file)
  })

using: https://www.npmjs.com/package/glob

then we could get all the manifest files in the cache.

@wardpeet

This comment has been minimized.

Copy link
Member

wardpeet commented Sep 25, 2019

@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.

@moonmeister

This comment has been minimized.

Copy link
Contributor

moonmeister commented Sep 26, 2019

What about caching the non-default manifests?

@wardpeet

This comment has been minimized.

Copy link
Member

wardpeet commented Sep 26, 2019

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

Copy link
Member

wardpeet left a comment

Let's ship this. Thanks @dkthehuman & @moonmeister

@wardpeet wardpeet merged commit f88a9e2 into gatsbyjs:master Sep 26, 2019
20 checks passed
20 checks passed
Danger All good
Details
Peril All green. Well done.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 67 tests passed in 00:27
Details
unit_tests_windows Build #20190925.53 succeeded
Details
@dkthehuman dkthehuman deleted the dkthehuman:update-manifest-on-nav branch Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.