search

pwa-builder / PWABuilder

The simplest way to create progressive web apps across platforms and devices. Start here. This repo is home to several projects in the PWABuilder family of tools.
https://docs.pwabuilder.com
Other
2.78k stars 287 forks source link

Android Package cannot find icons #731

Closed joel-44 closed 4 years ago

joel-44 commented 4 years ago

Describe the bug With the latest updates I noticed that I cannot create the android apk for applications built with nuxt and https://pwa.nuxtjs.org/. When trying to create them an error saying that icons are not found appears. The icons url seem to have added an extra "_nuxt/" path on the url.

The following error shows on the call to pwabuilder-cloudapk.azurewebsites.net Error generating signed APK: Error: Failed to download icon https://monstermock.com/_nuxt/_nuxt/icons/icon_512.cb990b.png. Responded with status 404

To Reproduce Steps to reproduce the behavior:

  1. Go to https://www.pwabuilder.com/
  2. Enter "https://monstermock.com" and Click on "Start"
  3. Click on "Build my PWA"
  4. Click on "Android"
  5. See error

Alternative do the same with "https://rescueapp.es" and you will see the icons failures but not the call to pwabuilder-cloudapk.azurewebsites.net being done

Expected behavior The android package is created without issues and with the correct URL path for the icons

Screenshots

Screenshot 2020-04-29 at 15 12 50

Additional context I am going to try to submit a PR with the fix myself

ghost commented 4 years ago

Hello joel-44, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will investigate the issue and help solve it ASAP. Other community members may also look into the issue and provide feedback 🙌

jgw96 commented 4 years ago

Hey, thanks for opening an issue with us and thanks for the PR! Before moving forward with the PR I just wanted to do a bit of debugging with you. https://pwabuilder.com is currently built with Nuxt and works great with our TWA generation service, so I want to understand what is different. When i got to your site and inspect it in Edge devtools I noticed that your manifest link also is not valid (although Edge is finding it somehow) image Where ours is a valid link https://www.pwabuilder.com/manifest.webmanifest . This seems to be the same issue with your icons. Any thoughts on this?

joel-44 commented 4 years ago

Hi! That's strange.. I can see the file correctly if I just go to https://monstermock.com/_nuxt/manifest.f333675d.json

and it also shows fine on the chrome dev tools

manifest

I think the main difference is that manifest on my cases is not added to the root but to /_nuxt/ folder (all handled by https://pwa.nuxtjs.org/). But I think the issue is not on the manifest as it is validated correctly but on assuming that if the manifest is under another path the images have to be prefixed with it as well (when on the case of the pwa plugin is not the case, the path is already added to the icons as well)

jgw96 commented 4 years ago

Thanks for the extra info! I agree that paths like this are something we should handle better, just not sure at the moment of the best way to do this as we have to make some assumptions about paths to make sure that the service works for 90% of web setups but have enough customization for the last 20%. I will do some thinking on this!

jgw96 commented 4 years ago

@TesheG or @JudahGabriel , any ideas here?

joel-44 commented 4 years ago

I think this can affect many websites as anyone using https://github.com/nuxt-community/pwa-module might get it.. I agree that it can be dangerous to touch paths as fixing it for some sites might break others, that's why on the PR I didn't go too deep on how the the paths are created and I just added a line to remove the duplication in case it happens (which I think it is quite safe)

JudahGabriel commented 4 years ago

So your PR fixes the symptom but not the root cause.

If I analyze monstermock.com in PWABuilder, then go to Manifest, I see the icons are all the wrong path there. Do we know why? That's what we should fix.

I'm guessing we have code somewhere that takes the URL of the manifest (e.g. /_nuxt/manifest.f333675d.json), looks at its icons (e.g. /_nuxt/icons/icon_64.cb990b.png) and combines the two: /_nuxt/_nuxt/icons/icon_64.cb990b.png - resulting in the bug.

@lee-leonardo want to have a look at this one?

joel-44 commented 4 years ago

yes, that's exactly the problem, with the latest releases on pwabuilder there was a change on how the path are constructed (it worked fine before) so the path for the manifest is added to the icons as well (even if they have it incorporated already) so it ends up being duplicated (in this case "_nuxt")

I assumed that there are some cases where this is needed, so I just made it to replace the duplication in case it happens.

The code that combines the two can be seen on my PR, it happens just above my change, and it those lines are called here: https://github.com/pwa-builder/PWABuilder/blob/master/store/modules/generator/generator.mutations.ts#L35

I think this if condition for the generatedUrl was added recently. I think my PR fixes the issue without breaking any purpose of that new code but tell me if you see a better solution for this

erkstruwe commented 4 years ago

I think the correct way to implement this was to resolve the icon's path from the manifest relative to the path of the manifest (that's also the way browsers do it). The URL constructor will do the trick:

new URL("/subfolder1/icon.png", "https://www.domain.com/subfolder1/manifest.json").href
// 'https://www.domain.com/subfolder1/icon.png'
new URL("/subfolder2/icon.png", "https://www.domain.com/subfolder1/manifest.json").href
// 'https://www.domain.com/subfolder2/icon.png'
new URL("subsubfolder/icon.png", "https://www.domain.com/subfolder1/manifest.json").href
// 'https://www.domain.com/subfolder1/subsubfolder/icon.png'
new URL("https://www.otherdomain.com/icon.png", "https://www.domain.com/subfolder1/manifest.json").href
// 'https://www.otherdomain.com/icon.png'
joel-44 commented 4 years ago

Edited, I read the comment above wrong, sounds good 👍

joel-44 commented 4 years ago

I adjusted the PR to use this solution instead. Tell me what you think!

JudahGabriel commented 4 years ago

Looks good to me. We'll give it a spin. Thanks for fixing this!

sbracegirdle commented 4 years ago

I discovered the same issue when testing a number of popular PWA examples. The issue seems related to whether image paths are resolved relative to the base path root (e.g. notarealsite.com/), or the document root (e.g. notarealsite.com/hosted/here/index.html). It should be the latter if the image path does not explicitly start with a forward slash.

For example; img/myimg.png should resolve to notarealsite.com/hosted/here/img/myimg.png, and not notarealsite.com/img/myimg.png.

It thought it may help you during your testing process to have these additional examples where I experienced the issue:

https://googlechromelabs.github.io/affilicats/ https://jakearchibald.github.io/svgomg/ https://mdn.github.io/pwa-examples/a2hs/ https://mdn.github.io/pwa-examples/js13kpwa/

JudahGabriel commented 4 years ago

Testing the above fix now...

JudahGabriel commented 4 years ago

Still broken, it appears the above fix didn't solve it.

@TesheG here's the repro:

  1. Go to pwabuilder.com
  2. Analyze https://jakearchibald.github.io/svgomg/
  3. Build My PWA
  4. Download Android package

You'll get a 404 error about a missing image.

JudahGabriel commented 4 years ago

@joel-44 This is now fixed. I just verified by following your repro steps, and I was able to download the Android package. Closing this issue. If you're still encountering an issue, please feel free to reopen.

JudahGabriel commented 4 years ago

@sbracegirdle I just verified it's fixed for https://jakearchibald.github.io/svgomg/ as well. 👍