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

Broken icon links in Manifest file when using assetPrefix #18497

Closed
ehannes opened this issue Oct 11, 2019 · 8 comments · Fixed by #20142
Closed

Broken icon links in Manifest file when using assetPrefix #18497

ehannes opened this issue Oct 11, 2019 · 8 comments · Fixed by #20142
Labels
help wanted Issue with a clear description that the community can help with. type: bug An issue or pull request relating to a bug in Gatsby

Comments

@ehannes
Copy link

ehannes commented Oct 11, 2019

Description

It seems that gatsby-plugin-manifest does not care about assetPrefix.

Steps to reproduce

  1. Create a new Gatsby project
  2. Add an assetPrefix
  3. Run gatsby build --prefix-paths and compare the icon links in public/index.html with the ones in manifest.webmanifest. Those in index.html are prefixed, while the ones in the manifest are not.

Se this repo: https://github.com/ehannes/broken-icon-links-in-manifest, and compare icon links in public/index.html with public/manifest.webmanifest.

Expected result

Icon links in webmanifest should be prefixed with assetPrefix, as they are in the built index.html.

Actual result

Gatsby prefixes the icons correctly, for instance href="/assets/icons/icon-48x48.png?v=4b11ef345c95e35b68f0d09fdabdc428"/>. But in the manifest, the links are missing the asset prefix: "src": "icons/icon-48x48.png?v=4b11ef345c95e35b68f0d09fdabdc428". This leads to the following error in the browser console:
Error while trying to use the following icon from the Manifest: http://localhost:9000/icons/icon-144x144.png?v=4b11ef345c95e35b68f0d09fdabdc428 (Download error or resource isn't a valid image).

This is probably connected to this change: cdd800f, where the manifest file were no longer prefixed.

Environment

$ gatsby info --clipboard

  System:
    OS: Linux 4.15 Pop!_OS 18.04 LTS
    CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
    Shell: 4.4.20 - /bin/bash
  Languages:
    Python: 2.7.15+ - /usr/bin/python
  Browsers:
    Chrome: 77.0.3865.90
    Firefox: 69.0.1
  npmPackages:
    gatsby: ^2.15.35 => 2.15.35
    gatsby-image: ^2.2.27 => 2.2.27
    gatsby-plugin-manifest: ^2.2.21 => 2.2.21
    gatsby-plugin-offline: ^3.0.14 => 3.0.14
    gatsby-plugin-react-helmet: ^3.1.11 => 3.1.11
    gatsby-plugin-sharp: ^2.2.29 => 2.2.29
    gatsby-source-filesystem: ^2.1.31 => 2.1.31
    gatsby-transformer-sharp: ^2.2.21 => 2.2.21
@ehannes
Copy link
Author

ehannes commented Oct 11, 2019

I guess it's gatsby-plugin-manifest/src/common.js that should take assetPrefix into consideration.

@LekoArts LekoArts added help wanted Issue with a clear description that the community can help with. type: bug An issue or pull request relating to a bug in Gatsby labels Nov 4, 2019
@LekoArts
Copy link
Contributor

LekoArts commented Nov 4, 2019

We'd be happy to receive a PR fixing this!

@github-actions

This comment has been minimized.

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Dec 1, 2019
@moonmeister moonmeister added effort: low not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Dec 1, 2019
@moonmeister
Copy link
Contributor

moonmeister commented Dec 3, 2019

So did a quick look on this. I initially tired adding prefixing to the common.js file. this way when the defaults were fetched they'd have the prefixes. However, because this isn't happening before gatsby is bootstrapping itself it breaks looking for the __PATH_PREFIX___ global variable.

It'd be simple enough to add a loop into the gatsby-node.js code to prefix the icons. However, this would possibly affect manually defined paths when working in "manual", and "hybrid" modes.

What do y'all think, should this only affect the default icon set in automatic mode, or should this also prefix developer defined paths? I can see positives of both, and downsides to both but am leaning towards prefixing everything.

The downsides to prefixing everything is well, developer puts in the full path and it gets prefixed again. The benefit is the prefix is always handled by config, developer puts in the short path and it gets prefixed, if the prefix ever changes the developer only changes the build/config.

Thoughts?

@Santos-Luis
Copy link

The issue is also happening with me. We have a separated cdn and we used 12128 to solve that problem. Although, the solution implemented does not work with the gatsby-plugin-manifest, since the in the sw.js file the array __precacheManifest has all the objects with the attribute url without the assetPrefix

@femesq
Copy link

femesq commented Jun 29, 2020

Is it working for anyone using with assetPrefix and gatsby build --prefix-assets?

Tried this code on gatsby-node.js:

exports.onPostBootstrap = async ({ basePath }) => {
  console.log("onPostBootstrap: ", basePath);
};

and basePath is '' - used here.

Thanks for any help on this!

@moonmeister
Copy link
Contributor

@femesq Is your problem related to #25207 ?

@femesq
Copy link

femesq commented Jun 30, 2020

@moonmeister Yes! Thanks for pointing me to there... will keep track for updates in that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issue with a clear description that the community can help with. type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants