Update Manifest gatherer to use gather error instead of -1 artifact #1624
Conversation
brendankenny
force-pushed
the
manifest
branch
from
b4dcabe
to
8d29e15
Compare
| } | ||
|
|
||
| return Manifest._errorManifest(errorString); | ||
| // The driver will return an empty string for url and the data if |
There was a problem hiding this comment.
i'd keep this comment above the if (response.url) conditional
There was a problem hiding this comment.
hopefully clarified the comment
| * Returns the parsed manifest or null, if the page had no manifest. | ||
| * @param {!Object} options | ||
| * @return {!Promise<?Manifest>} | ||
| */ | ||
| afterPass(options) { | ||
| return options.driver.sendCommand('Page.getAppManifest') | ||
| .then(response => { | ||
| // We're not reading `response.errors` however it may contain critical and noncritical |
There was a problem hiding this comment.
wdyt about handling these? it might help us differentiate between bad JSON, missing, or other issues.
There was a problem hiding this comment.
wdyt about handling these?
do we have a bug tracking it? I assume you added that comment :)
it might help us differentiate between bad JSON, missing, or other issues.
the first two should be handled, but yeah, other issues would be good to catch
There was a problem hiding this comment.
do we have a bug tracking it? I assume you added that comment :)
lookie lookie: #823
OHHHHHHHHHHHHHHHHHH WHAT NOW
| const cacheContents = artifacts.CacheContents; | ||
|
|
||
| if (!(manifest && manifest.start_url && manifest.start_url.value)) { | ||
| if (!artifacts.Manifest || !artifacts.Manifest.value) { |
There was a problem hiding this comment.
Since artifacts.Manifest is now null in the non-exist case, should we just change all these to this?
if (artifacts.Manifest === null) {There was a problem hiding this comment.
this is checking two error states.
-
no manifest, so the
artifacts.Manifestisnull. This could do the explicit check, but not sure if it's worth it as the gatherer will only returnnullor an object, so only null will be falsy. -
invalid JSON, so
artifacts.Manifest.valuewill beundefined. Invalid JSON is the only way thatartifacts.Manifest.value, but we have to check for it.One solution I considered to get rid of the extra check was to throw if the JSON was invalid, but it felt a bit weird to declare a gatherer error when it really was an error in the user's data. It would also mean that every manifest audit would have a
debugStringofAudit error: Manifest gatherer encountered an error: invalid JSON(or whatever) . That's a lot of visual noise when it could just be on themanifest-existsaudit as it is here. OTOH, invalid JSON in your manifest is a pretty big and obvious error and one the site owner should fix quickly, so maybe that's fine.
There was a problem hiding this comment.
well i guess i'm saying we should handle the invalid JSON case via response.errors in the gatherer, so we can be more clear here. otherwise these are the only audits that have to differentiate between two error states (plus they can't say which they're in)
There was a problem hiding this comment.
Hmm, maybe I don't understand what you're saying.
Just to be clear, by invalid JSON I mean just unparseable JSON; if the manifest is parseable by JSON.parse but is invalid according to the manifest spec, that will show up in the individual audits.
This code can separate these two states, but I put them in one conditional on purpose because we want the audits to just fail and not output a debugString (otherwise we'd have 12 debugStrings or whatever in the report saying the same thing).
| @@ -40,6 +40,13 @@ class ManifestExists extends Audit { | |||
| * @return {!AuditResult} | |||
| */ | |||
| static audit(artifacts) { | |||
| if (!artifacts.Manifest) { | |||
There was a problem hiding this comment.
compare against null?
as above, we can compare against null if you feel strongly about it, but null is the only falsy value you could get here (and if we added the check against null specifically and let through the other falsy values, artifacts.Manifest.value below would throw anyways)
brendankenny
force-pushed
the
manifest
branch
from
8d29e15
to
6076a56
Compare
There was a problem hiding this comment.
We talked this one out a bit. A few more things but i trust it'll be good.
- Adopt Page.getAppManifest's errors object #1666 was filed to track using the errors property
- Need comment clarifying the null and undefined values on the artifact and what they mean, despite the audits loosely looking for falsy
Part of #941. There a lot of audits that depend on the manifest gatherer, so separated this one from #1623.
The most impactful change I made was to make the manifest gatherer return
nullif the page has no manifest as it seems appropriate to have an explicit value for this (and obviously not having a manifest is not an error).This sticks to the minimal redundancy of
debugStrings that #1598 started: if the page has no manifest, all the manifest audits will fail with no message. If the manifest was there but invalid JSON, they will all fail but onlymanifest-existswill report thedebugString.There's a good bit of redundancy here. It may make sense to split these into their own
audits/subdirectory a laaccessibility/and do some kind of base class thing (and unify testing...most of these tests are duplicates with only the tested property changed)