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

Question: Nested paths breaks lighthouse PWA test #23

Closed
userquin opened this issue Feb 6, 2021 · 27 comments
Closed

Question: Nested paths breaks lighthouse PWA test #23

userquin opened this issue Feb 6, 2021 · 27 comments

Comments

@userquin
Copy link
Member

userquin commented Feb 6, 2021

I have found some problems when using nested routes with the plugin: lighthouse cannot resolve start_url when in nested path route, for example just request https://vitesse.netlify.app/hi/a in private mode on chrome, open dev tools and run lighthouse with PWA check checked:

imagen

The problem in question can be solved just adding the scope to the manifest.webmanifest file and also initializing it with the basePath.

By default it is initialized as ./, and I don't know if this value is ok to be used as scope when registering the service worker. I register it using router.isReady callback instead on index page and change the default pattern to be /, and so the header, the manifest.webmanifest and the registration matches:

imagen

This is because it seems that the nested parent path acts as a new scope, that is distinct from the missing scope.

Another think that I don't know if it is required is to add a header like that to the html (I just add it as a http header from the server side):

service-worker-allowed: `${basePath}`

Here is a working manifest.webmanifest.

You can see a working sample here: https://cancer.malvarez.info/conozca-su-riesgo, then click on + icon and run the test (it is with vite 1 + vitesse 1, I'm migrating it to vite 2 + vitesse 2).

@userquin
Copy link
Member Author

userquin commented Feb 6, 2021

@antfu antfu closed this as completed in d351102 Feb 6, 2021
@antfu
Copy link
Member

antfu commented Feb 6, 2021

Can you verify if v0.4.5 fixes your problem? Thanks.

@userquin
Copy link
Member Author

userquin commented Feb 6, 2021

If I remove start_url and scope from vite.config.ts, the generated manifest.webmanifest file has no the scope...

imagen

with generated file:

imagen

F:\work\projects\quini\vite-vitesse-2\dssa-cancer-mama-ui-v2>pnpm upgrade vite-plugin-pwa
 WARN  @intlify/vite-plugin-vue-i18n: @rollup/pluginutils@4.1.0 requires a peer of rollup@^1.20.0||^2.0.0 but none was installed.
 WARN  vite-plugin-pages@0.1.9 requires a peer of vite@^2.0.0-beta.61 but version 2.0.0-beta.46 was installed.
Packages: +1 -1
+-
Progress: resolved 701, reused 700, downloaded 1, added 1, done
 WARN  Failed to find "/fsevents/2.1.3" in lockfile during hoisting. Next aliases will not be hoisted: fsevents

devDependencies:
- vite-plugin-pwa 0.4.4
+ vite-plugin-pwa 0.4.5

@userquin
Copy link
Member Author

userquin commented Feb 6, 2021

@antfu I have not marked this as a bug, in fact I can it make work just configuring it on the configuration file: there is no need to change the code... just was an enhacement, add some hints on the README.md file and done.

@antfu
Copy link
Member

antfu commented Feb 6, 2021

The scope would be set to base by default from v0.4.6. I think it's good to make things just works if possible, thanks for the notice and suggestions

@userquin
Copy link
Member Author

userquin commented Feb 6, 2021

@antfu do you deploy the demo to netifly when versioning? demo on netifly still with missing scope on manifest file

I have tested that v0.4.6 generates the scope correctly on pages (inlined) and registerWS.js (not inlined) and manifest.webmanifest.

@antfu
Copy link
Member

antfu commented Feb 6, 2021

What do you mean demo? Vitesse? That's a separate repo and the deps need to update manually (where I often do it once a while)

@userquin
Copy link
Member Author

userquin commented Feb 6, 2021

yes, upps, sorry

@userquin
Copy link
Member Author

userquin commented Feb 6, 2021

when you redeploy it, just test the dynamic page...

@userquin
Copy link
Member Author

userquin commented Feb 6, 2021

@antfu I think it will need the header to make it work.

@userquin
Copy link
Member Author

@antfu After some review (vitesse demo) it seems that PWA with dynamic pages are incompatible: a PWA must have an URL for each page, so if we map a page to be dynamic then we got that the service worker cannot resolve it, it is dynamic, and then on ligthhouse we got:

imagen

You can see that there is a Check List that ligthhouse doesn't check, it must be done manually:

imagen

@hannoeru
Copy link
Collaborator

Dynamic route should fixed in v0.4.2, and start_url is a lighthouse issue #20

@userquin
Copy link
Member Author

@hannoeru can you test vitesse root page demo on incognito mode with Chrome Beta?

I'm testing my app with chrome 88 and I have not this issue in any page.

@userquin
Copy link
Member Author

I think this is not a lighthouse bug, it is something related to the configuration:

imagen

@userquin
Copy link
Member Author

while the root page of vitesse demo (there is no dynamic page):

imagen

@userquin
Copy link
Member Author

I don't understand what's happening. I create a simple test-app.js (code below) configuring node with node-static to serve dist folder and just works, so I don't know why is failing on netlify.

Just running node test-app-js:

imagen

/* eslint-disable */
'use strict'

const http = require('http')
const https = require('https')
const fs = require("fs");
const path = require('path')
const nStatic = require('node-static')

const fileServer  = new nStatic.Server('./dist')

const hostname = '192.168.1.37'
const port = 443

const httpsRedirect = async(req, res) => {
  res.writeHead(301, { "Location": `https://${hostname}:${port}${req.url}` });
  res.end()
}

const secureServer = https.createServer({
  ca: fs.readFileSync('192.168.1.37_allowed_cas.crt'),
  key: fs.readFileSync('192.168.1.37.key'),
  cert: fs.readFileSync('192.168.1.37.crt'),
}, (req, res) => {
  fileServer.serve(req, res)
})

secureServer.listen(443, () => {
  console.log(`El servidor se está ejecutando en https://${hostname}:${port}/`)
})

const redirectServer = http.createServer(httpsRedirect)

redirectServer.listen(80, () => {
  console.log(`El servidor se está ejecutando en http://${hostname}:${80}/`)
})

@userquin
Copy link
Member Author

Something is wrong @antfu , go to vitesse demo page, then open manifest from devtools, it is base64 encoded.

You must configure netlify to server manifest.webmanifest files with application/manifest+json content type, it is being served with application/octet-stream content type.

imagen

@userquin
Copy link
Member Author

Add this to netlify.toml:

[[headers]]
  for = "/manifest.webmanifest"
  [headers.values]
    Content-Type = "application/manifest+json"

@hannoeru
Copy link
Collaborator

hannoeru commented Feb 10, 2021

That didn't fixed the start_url issue, antfu-collective/vitesse#54 PR preview:
https://deploy-preview-54--vitesse.netlify.app/

@userquin
Copy link
Member Author

WTF ;) I'm missing something...

@userquin
Copy link
Member Author

Who is setting x-robots-tag: noindex?

@userquin
Copy link
Member Author

This prevent to service worker to download the PWA.

Open preview url, open devtools, go to application, remove storage including indexeddb and service workers.

If you press F5, then only workbox and sw scripts downloaded, the rest of the app is not downloaded...

So when testing, just didn0t work, all files are missing from the cache.

@userquin
Copy link
Member Author

If you open the same app on dev, you will see that PWA is downloaded:

Fron localhost via node-static:

imagen

The same from your preview url:

imagen

@userquin
Copy link
Member Author

@hannoeru

@userquin
Copy link
Member Author

forget previous comments, it seem it is another ghost..

@userquin
Copy link
Member Author

can update netlify.toml to include expiration and immutable entries to the manifest.webmanifest entry? It is served with cache-control: public, max-age=0, must-revalidate and age: 2035.

@userquin
Copy link
Member Author

also forgot last comment, tested on local and works as expected with those headers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants