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

pipe manifest fetch/parse error strings up for display #102

Merged
merged 3 commits into from
Mar 31, 2016
Merged

pipe manifest fetch/parse error strings up for display #102

merged 3 commits into from
Mar 31, 2016

Conversation

brendankenny
Copy link
Member

fixes #99

@brendankenny
Copy link
Member Author

for example:

Lighthouse results: https://flipkart.com
Will Get Add to Homescreen Prompt: 17%
 -- Has a Service Worker registration: false
 -- Manifest exists: false
    Unable to fetch manifest at /manifest.json: 404: Not Found.
 -- Contains background_color: false
 -- Contains icons: false
 -- Contains 192px icons: false
 -- Contains short_name: false
 -- Site has a viewport meta tag: true

@@ -47,7 +47,9 @@ class ManifestExists extends Audit {
*/
static audit(artifacts) {
return ManifestExists.generateAuditResult(
typeof artifacts.manifest !== 'undefined'
typeof artifacts.manifest.value !== 'undefined',
undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this undefined arg?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the audit's rawValue, whatever that is. It could still be a boolean in this case, but then it'll be displayed twice with the current cli pretty printing.

The only place this is used currently is in the FMP output, which puts the actual time in parentheses. There's probably a better way of piping that through

@paulirish
Copy link
Member

Yeah I like this setup, though I could prefer errorString as debugString.

};
})
.catch(reason => {
return this._errorManifest(`Unable to fetch manifest at ${manifestURL}: ${reason}.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we still need to add tests around these error messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a simple pass-through check. The more involved tests will be on the gatherer itself.

@brendankenny
Copy link
Member Author

added debugString output to the extension as well. PTAL

@paulirish
Copy link
Member

I was thinking that the property is always called debugString throughout the app, instead of using warning etc. Is that worth it?

@paulirish
Copy link
Member

aside from this silly naming issue, lgtm, obviously.

@brendankenny
Copy link
Member Author

whoops, didn't notice your last comment. Don't feel strongly, so went with being consistent on using debugString.

PTAL

@brendankenny brendankenny changed the title pipe manifest fetch/parse error strings back to cli pipe manifest fetch/parse error strings back to aggregator Mar 31, 2016
@brendankenny brendankenny changed the title pipe manifest fetch/parse error strings back to aggregator pipe manifest fetch/parse error strings up for display Mar 31, 2016
@paulirish
Copy link
Member

groovy. thanks amigo.

@paulirish paulirish added the +1 label Mar 31, 2016
return {
value: value,
rawValue: rawValue,
value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally i really hate mixing the : syntax it is very confusing ... but maybe it is something to get used to for me.

@samccone samccone added the +2 label Mar 31, 2016
@samccone samccone merged commit d1b8cfe into GoogleChrome:master Mar 31, 2016
@brendankenny brendankenny deleted the errorstring branch April 15, 2016 23:42
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

Successfully merging this pull request may close these issues.

Graceful recovery when no manifest is present
3 participants