Skip to content

Commit

Permalink
don't localize main manifest ~ fix sw integration
Browse files Browse the repository at this point in the history
  • Loading branch information
@wardpeet
wardpeet committed Sep 24, 2019
1 parent cac3354 commit 626efed
Showing 1 changed file with 13 additions and 7 deletions.
20 changes: 13 additions & 7 deletions packages/gatsby-plugin-manifest/src/gatsby-node.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,20 +84,26 @@ exports.onPostBootstrap = async (
cacheModeOverride = { cache_busting_mode: `name` }
}

return makeManifest(cache, reporter, {
...manifest,
...locale,
...cacheModeOverride,
})
return makeManifest(
cache,
reporter,
{
...manifest,
...locale,
...cacheModeOverride,
},
true
)
})
)
}
activity.end()
}

const makeManifest = async (cache, reporter, pluginOptions) => {
const makeManifest = async (cache, reporter, pluginOptions, shouldLocalize) => {
const { icon, ...manifest } = pluginOptions
const suffix = pluginOptions.lang ? `_${pluginOptions.lang}` : ``
const suffix =
shouldLocalize && pluginOptions.lang ? `_${pluginOptions.lang}` : ``

This comment has been minimized.

Sign in to view

Copy link
@moonmeister

moonmeister Sep 24, 2019

Contributor

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.

Sign in to view

Copy link
@moonmeister

moonmeister Sep 25, 2019 •
edited
Loading

Contributor

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

// Delete options we won't pass to the manifest.webmanifest.
delete manifest.plugins
Expand Down

0 comments on commit 626efed

Please sign in to comment.