-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Collapse the 9 manifest PWA audits into 3 #1847
Conversation
@@ -53,11 +43,15 @@ class ServiceWorker extends Audit { | |||
static audit(artifacts) { | |||
// Find active service worker for this URL. Match against | |||
// artifacts.URL.finalUrl so audit accounts for any redirects. | |||
const version = getActivatedServiceWorker( | |||
artifacts.ServiceWorker.versions, artifacts.URL.finalUrl); | |||
const versions = artifacts.ServiceWorker.versions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No differences here, right? Just small refactor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. no difference.
* * Site engagement score of 2 or higher | ||
|
||
* This audit covers these requirements with the following exceptions: | ||
* * it doesn't look at standalone/fullscreen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? just still working on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because #495 and https://twitter.com/triblondon/status/838966967604142080 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll bring this topic back up on the recent thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems wrong to not validate something which is in fact a real world requirement.
Maybe now would be a good time to resolve the question of whether Lighthouse is an expression of Chrome's policy, in which case I'm keen to continue to use this forum to advocate for the requirements to change to be in line with web standards, or if it is not, append "... in Google Chrome" to the end of the title of this audit (and I'll toddle off to file a bug in Chrome!).
I'm hopeful of convincing Samsung to drop the fullscreen/standalone requirement for an install prompt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @paulirish thank you for taking note of this point and paying attention to the issue. Appreciated.
009853b
to
0f76906
Compare
@paulirish can you rebase? |
0f76906
to
616d482
Compare
Good news, this PR drops 500 lines from the code base. :) I have updated this PR's top description with a full description of what's happening here. Only thing remaining on my list is sorting out the smoke tests, but I should have that done within the hour. |
smoke tests are sorted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
manifestValues.allChecks | ||
.filter(item => splashScreenCheckIds.includes(item.id)) | ||
.forEach(item => { | ||
if (item.passing === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: !item.passing
. all of your toPass
s return bools, ya?
const failures = []; | ||
|
||
return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => { | ||
// 1: validate manifest is in order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comment? there's only step and the func name is obvi
if (failures.length > 0) { | ||
return { | ||
rawValue: false, | ||
debugString: `Unsatisfied requirements: ${failures.join(', ')}.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look ok when there are a number of failures? Should we line separate?
return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => { | ||
// 1: validate manifest is in order | ||
ThemedOmnibox.assessManifest(manifestValues, failures); | ||
// 2: validate we have a SW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate we have a theme color
/** | ||
* Returns results of all manifest checks | ||
* @param {Manifest} manifest | ||
* @return {<{isParseFailure: !boolean, parseFailureReason: ?string, allChecks: !Array}>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be {{isParseFailure: !boolean, parseFailureReason: ?string, allChecks: !Array}}
?
im no closure annotations guru.
let parseFailureReason; | ||
|
||
if (manifest === null) { | ||
parseFailureReason = 'Manifest is available'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Manifest is unavailable'? Maybe better: 'Manifest could not be parsed'
parseFailureReason = 'Manifest is available'; | ||
} | ||
if (manifest && manifest.value === undefined) { | ||
parseFailureReason = 'Manifest is parsed as valid JSON'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm confused. The strings don't describe failure conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is good feedback. I'm going to flip the language in the UI, too.
Failures: Manifest failed to parse as valid JSON. Manifest does not contain a start_url
formatter: Formatter.SUPPORTED_FORMATS.NULL | ||
}; | ||
|
||
// If we fail, share the failures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the last part of all these audits is the same. I wonder if there's a way to dry that up a bit (possibly other duplicated places). One idea is for these new audits to extend a base audit like Rob did for the a11y audits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg. done.
pinging @paulirish |
category: 'PWA', | ||
name: 'splash-screen', | ||
description: 'Configured for a custom splash screen', | ||
// When your app launches from a user\'s homescreen, the browser ' 'uses `background_color` to paint the background of the browser ' +'while your app loads for a smooth transition experience. ' + '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/manifest-contains-background_color).', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the browser ' 'uses
?
static audit(artifacts) { | ||
const failures = []; | ||
|
||
return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts on having these share a base class that does this plumbing and calls assessManifest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. just did this. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well. no call directly into assessManifest, as i'm leaving the base audit to be fairly agnostic.
{ | ||
id: 'hasStartUrl', | ||
userText: 'Manifest contains `start_url`', | ||
toPass: manifest => !!manifest.value.start_url.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚲 🏚 nit: feels super audit-y to phrase hasStartUrl
as passing in the gatherer since all of these are just facts we're checking for, what about validate
/isTrue
/something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like validate
. sgtm.
159e809
to
cf3a9fa
Compare
bd6bfd1
to
9741f53
Compare
@ebidel @patrickhulce thanks much for the review! Updated. Base class done. Inverted the language. Properties renamed for clarity. |
@@ -0,0 +1,59 @@ | |||
/** | |||
* @license Copyright 2017 Google Inc. All Rights Reserved. | |||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, I see what you're up to
} | ||
|
||
/** | ||
* @param {!{manifestValues: !Object, failures: !Array<!string>, themeColor: ?string}} result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove manifestValues
and themeColor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are sometimes provided. I updated to clarify they are nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine by me. We should get this in soon so it can unblock things for me to work on :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't notice you already submitted sometime halfway through this, d'oh. Sorry I took so long to get to this.
Didn't actually look at the manifest checks and if they're the same as before, but I guess I'll stop now :S
@@ -35,45 +35,44 @@ class ManifestShortNameLength extends Audit { | |||
}; | |||
} | |||
|
|||
|
|||
static assessManifest(manifestValues, failures) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not meant to be here?
@@ -0,0 +1,59 @@ | |||
/** | |||
* @license Copyright 2017 Google Inc. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were going with all or nothing for the license change
* @return {!AuditResult} | ||
*/ | ||
static audit(artifacts) { | ||
return Promise.resolve(this.audit_(artifacts)).then(result => this.createAuditResult(result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just make audit_
return a promise? then it's return this.audit_(artifacts).then(...)
. Or do Promise.resolve().then(_ => this.audit_(artifacts)).then(...)
to make sure errors are caught
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can also drop the arrow function, .then(this.createAuditResult);
since it's static
} | ||
|
||
/** | ||
* @param {!{failures: !Array<!string>, themeColor: ?string, manifestValues: ?Object, }} result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't use themeColor
or manifestValues
so can drop from here (which also helps it be a better generic "multi audit" audit base class). @param {{failures: !Array<string>}} result
|
||
'use strict'; | ||
|
||
const Audit = require('./multi-check-audit'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should go with MultiCheckAudit
so the class hierarchy is clear
return 'ManifestValues'; | ||
} | ||
|
||
static get validityIds() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used anywhere?
/** | ||
* Returns results of all manifest checks | ||
* @param {Manifest} manifest | ||
* @return {{isParseFailure: !boolean, parseFailureReason: ?string, allChecks: !Array}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a little long but handy to document the checks type: @return {{isParseFailure: boolean, parseFailureReason: (string|undefined), allChecks: !Array<{id: string, failureText: string, passing: boolean}>}}
} | ||
|
||
// manifest is valid, so do the rest of the checks | ||
const remainingChecks = ManifestValues.manifestChecks.map(item => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since all uses of the checks access them strictly by id
, does it make sense to switch to an object (with the current id
s as the keys) instead of an array?
// 8.10(5) | ||
it.skip('falls back to document URL and issues a warning for an invalid URL', () => { | ||
it('falls back to document URL and issues a warning for an invalid URL', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, one less skip :)
/* eslint-env mocha */ | ||
describe('PWA: splash screen audit', () => { | ||
describe('basics', () => { | ||
it('fails if page had no manifest', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to move these to be true unit tests, so that the MultiCheckAudit
subclasses were just getting back a set of checks with .passing
set to true or false, and the unit test could just check if they respond to the correct passing values. Then the actual manifest verification stuff would live in the test for the manifest computed artifact, where the code for that actually lives.
Would also cut down on some of these duplicated tests (e.g. the one off cases) and minimize things like the manifest parser setup even more
@brendankenny thx! just filed #1920 to track these. Also!! The next ~100 PRs and issues get pretty memorable IDs, so let's make em count! :) |
updated everything here march 22
Instead of relying on topic grouping, this changes the {add to homescreen, splash screen, omnibox theming} audits to be all or nothing. (Scoring wise, if you're missing manifest
short_name
, you shouldn't really be getting points for the other manifest items.)Screenshot of the PWA audits now:
![image](https://cloud.githubusercontent.com/assets/39191/24228691/0de9f5d8-0f33-11e7-8b85-1556f07f0705.png)
(I stripped out the topics for readability as the double nesting is a little funny)
Functional changes
display
in a2hs/install-banner is back! Based on the resolution in Remove standalone / fullscreen requirement #495 (comment), we accept standalone/fullscreen/minimal-ui.pwa.rocks
for example fails now.name
isn't accepted as a fallback ifshort_name
is missing any longer. Some sites like svgomg fail on this now.Technical changes
manifestValues
).audits/manifest-*.js
have been nuked. (short-name-length remains).manifestValues
tests or the PWA audits tests.PTAL!