Viewer: add sw for ATHS #1571
Viewer: add sw for ATHS #1571
Conversation
|
cc @XhmikosR |
|
Pardon my ignorance but what dones ATHS stand for? Apart from that the changes look good. I was in the verge when I added manifest.json if it should be in root or not, but agreed this is simpler. |
|
Ah. Short for "Add to Homescreen" |
|
You guys make acronyms out of everything :P |
| @@ -43,3 +43,10 @@ Promise.all(loadPolyfillPromises).then(_ => { | |||
| // eslint-disable-next-line no-new | |||
| new LighthouseViewerReport(); | |||
| }); | |||
|
|
|||
| if ('serviceWorker' in navigator) { | |||
| navigator.serviceWorker.register('/sw.js', {scope: './'}).catch(e => { | |||
There was a problem hiding this comment.
isn't that scope implied? (iirc, the default scope matches the path of the SW resource?)
There was a problem hiding this comment.
I've been told to ALWAYS use scope. @jeffposnick says so!
| @@ -21,7 +21,7 @@ | |||
| <meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1"> | |||
| <title>Lighthouse Report Viewer</title> | |||
| <link rel="shortcut icon" href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAADjklEQVR4AWI08P/HQEvAQrxSQKvlECfLFYXx75xCY2qmh89GbNvOMjb3v9jOOlxnFWxj206ebQ3b7q6q+z1rNagu8/zvPSZACAABpeUAA0miMgU7SA7JjCraFGwZwECOwvL75dWjsKgWBKtx0jvWo+vkBAFbACCkByMP6nMn48+AVgXB2fzSCwsv22/lMGlUhmJ0AE7BH8dyUUDbUEgN6RzJRSeaPxhdRYR0Inel+7Hd5lBiFpkMAxACc0394//9C4voFHDiAAGLpuOXebdfdHfctgwJKaZRLRKy6ItrSis6RBnVBgGtbHyKTEmJHQoEXoBCE5BCrDeA2ogMUIGDAKEBDEhUqwgMqBYDjW4DQzmuffVdqff42/ZQYYqVcMXGZsMPyCsH3lyJSetxvEaxAQXdjR1HjfwCdIS7lo2DZke26Qe+MXO12OWkGT0O6oE7vMGkMnkYw4aN1KQgMKExhXqswfiov4+a7MQ11XPnbr/5qpKlgACAAQj94Lu271bN9DUecQasIZlNzG72llRAAKJiAi+/BSHrSFjRvQhg3DEKEqJh08tsmLTx597+f6enr4cc2Zpk57pihfX24dW7RHcOLLUbJYhJSl0ErQCI9BVXH/XrO97QasuvQQSiECa0BrQCIIJp6X9T/r8QG6L71WYSqCoIIGo2BZDUBnS/D9EA9Nun1iYvbM0MFExIDQRoKFatc1Z6zrm5uWeObJotq0BGV9FuQBWq5a4Fw3PPz848rZHstZSuA5FWAFSMP2nOppOOGpl6qh9PCSg0IFyHKjSQyDNQHTru2t75NOEe0fsf246oAmFkI6vCdnWvbQFQFCKx8vCswV8TrDLiDLgH4Nr7RAtNsrC9d8sfk7b8ls4igdNy8CQKAISlsB0FjZfd3Lfp155tf8fKI4BxZZIj/oTdVEAIAcJFOCmzauHG71I7/rdreUAgAqpDP05fDARCAQQARwEIBQSVxq0FyaLvZZtevpHa8WHw8cft6cpxlq8eAJtIhnSbWDf951yx3y13OqUuu5qyGgkxCgGFh9cDihDGbTa6BqvT1lWmrav3bmt2ZMJ4mU6TGgIC4DBzcv/JqAau1WhzSt3x9Ixk/4Jk/8J4ZrrViFMA4W6A7+WK8xcVjvyrOmVD0FbAXokcT48r+xVqLKvuJYbmpNadnlp3mpufJHOe/GXktM+r09bT8kEdq9BRYAbGSgzP7ll82U71Mc+ZFooXgwAAAABJRU5ErkJggg=="> | |||
| <link rel="manifest" href="images/manifest.json"> | |||
| <link rel="manifest" href="manifest.json"> | |||
There was a problem hiding this comment.
can you throw the manifest link above the favicon reference?
IMO makes sense before, based on priority.
| if ('serviceWorker' in navigator) { | ||
| navigator.serviceWorker.register('/sw.js').catch(e => { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Error during service worker registration:', e); |
There was a problem hiding this comment.
actually, we probably want to register this at ${location.pathname}sw.js so it is restricted to googlechrome.github.io/lighthouse/viewer/ and also works on localhost.
There was a problem hiding this comment.
navigator.serviceWorker.register('sw.js') for that matter.
|
PTAL for sanity |
| @@ -0,0 +1 @@ | |||
| // Intentionally empty for now. | |||
|
|
||
| if ('serviceWorker' in navigator) { | ||
| navigator.serviceWorker.register('sw.js').catch(e => { | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
since this file is only for running in the browser, want to just do /* eslint-disable no-console */ at the top of the file? Or is it better to keep it around to cut down on console calls left over from development?
(the rule is a bit annoying for debugging in viewer and in the extension since both have the gulp lint task as a required prerequisite for gulp build, but it's not the end of the world)
There was a problem hiding this comment.
Just removed the .catch for now. We don't care if this shell sw fails to register.
R: all
Follow up on #1554. Adds a shell sw so ATHS works.