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

core: remove last debugString references #5856

Merged
merged 4 commits into from
Aug 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions lighthouse-core/audits/webapp-install-banner.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,14 +104,13 @@ class WebappInstallBanner extends MultiCheckAudit {

if (!hasOfflineStartUrl) {
failures.push('Service worker does not successfully serve the manifest\'s start_url');
// TODO(phulce): align gatherer `debugString` with `explanation`
if (artifacts.StartUrl.debugString) {
failures.push(artifacts.StartUrl.debugString);
if (artifacts.StartUrl.explanation) {
failures.push(artifacts.StartUrl.explanation);
}
}

if (artifacts.StartUrl.debugString) {
warnings.push(artifacts.StartUrl.debugString);
if (artifacts.StartUrl.explanation) {
warnings.push(artifacts.StartUrl.explanation);
}

return {failures, warnings};
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/gatherers/manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const BOM_FIRSTCHAR = 65279;
class Manifest extends Gatherer {
/**
* Returns the parsed manifest or null if the page had no manifest. If the manifest
* was unparseable as JSON, manifest.value will be undefined and manifest.debugString
* was unparseable as JSON, manifest.value will be undefined and manifest.warning
* will have the reason. See manifest-parser.js for more information.
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['Manifest']>}
Expand Down
22 changes: 11 additions & 11 deletions lighthouse-core/gather/gatherers/start-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,23 @@ class StartUrl extends Gatherer {
.then(manifest => {
const startUrlInfo = this._readManifestStartUrl(manifest);
if (startUrlInfo.isReadFailure) {
return {statusCode: -1, debugString: startUrlInfo.reason};
return {statusCode: -1, explanation: startUrlInfo.reason};
}

return this._attemptManifestFetch(passContext.driver, startUrlInfo.startUrl);
}).catch(() => {
return {statusCode: -1, debugString: 'Unable to fetch start URL via service worker'};
return {statusCode: -1, explanation: 'Unable to fetch start URL via service worker'};
});
}

/**
* Read the parsed manifest and return failure reasons or the startUrl
* @param {?{value?: {start_url: {value?: string, debugString?: string}}, debugString?: string}} manifest
* @param {?{value?: {start_url: {value?: string, warning?: string}}, warning?: string}} manifest
* @return {{isReadFailure: true, reason: string}|{isReadFailure: false, startUrl: string}}
*/
_readManifestStartUrl(manifest) {
if (!manifest || !manifest.value) {
const detailedMsg = manifest && manifest.debugString;
const detailedMsg = manifest && manifest.warning;

if (detailedMsg) {
return {isReadFailure: true, reason: `Error fetching web app manifest: ${detailedMsg}`};
Expand All @@ -50,12 +50,12 @@ class StartUrl extends Gatherer {
}

// Even if the start URL had an error, the browser will still supply a fallback URL.
// Therefore, we only set the debugString here and continue with the fetch.
if (manifest.value.start_url.debugString) {
return {isReadFailure: true, reason: manifest.value.start_url.debugString};
// Therefore, we only set the warning here and continue with the fetch.
if (manifest.value.start_url.warning) {
return {isReadFailure: true, reason: manifest.value.start_url.warning};
}

// @ts-ignore - TODO(bckenny): should actually be testing value above, not debugString
// @ts-ignore - TODO(bckenny): should actually be testing value above, not warning
return {isReadFailure: false, startUrl: manifest.value.start_url.value};
}

Expand All @@ -64,13 +64,13 @@ class StartUrl extends Gatherer {
* Resolves when we have a matched network request
* @param {Driver} driver
* @param {string} startUrl
* @return {Promise<{statusCode: number, debugString: string}>}
* @return {Promise<{statusCode: number, explanation: string}>}
*/
_attemptManifestFetch(driver, startUrl) {
// Wait up to 3s to get a matched network request from the fetch() to work
const timeoutPromise = new Promise(resolve =>
setTimeout(
() => resolve({statusCode: -1, debugString: 'Timed out waiting for fetched start_url'}),
() => resolve({statusCode: -1, explanation: 'Timed out waiting for fetched start_url'}),
3000
)
);
Expand All @@ -88,7 +88,7 @@ class StartUrl extends Gatherer {
if (!response.fromServiceWorker) {
return resolve({
statusCode: -1,
debugString: 'Unable to fetch start URL via service worker',
explanation: 'Unable to fetch start URL via service worker',
});
}
// Successful SW-served fetch of the start_URL
Expand Down
56 changes: 28 additions & 28 deletions lighthouse-core/lib/manifest-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,21 @@ const ALLOWED_ORIENTATION_VALUES = [
*/
function parseString(raw, trim) {
let value;
let debugString;
Copy link
Member

Choose a reason for hiding this comment

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

seems like we want the manifest-parser still returning something like debugString? explanation seems too generic for what is always indicating an error/warning. Gatherers that use it would still only use the standard thing

Copy link
Member

Choose a reason for hiding this comment

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

Originally in manifest-parser it was warning, which got switched to debugString, which was then spread throughout Lighthouse cause why not, all in #102 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

explanation seems too generic for what is always indicating an error/warning.

sounds like you're upset with our decision to use explanation period :)

I'm not 100% sure what you're advocating for, is it for this PR to just remove the TODO and fix the tests then since we don't want to pursue it?

Copy link
Member

Choose a reason for hiding this comment

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

sounds like you're upset with our decision to use explanation period :)

haha, well, explanation on an audit is a little confusing in isolation (we really should add a jsdoc description to it in audit.d.ts :), but it comes in the context of a failing audit.

In this case the only indication that there was an issue is the existence of the string. I meant just for manifest-parser.js, it's not in the gatherer or audit pipelines, it's just a lib file, so it seems fine to have a more descriptive property name specific to that file.

Copy link
Member

Choose a reason for hiding this comment

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

looked and within manifest-parser, i'd say that, semantically, all these properties are really warnings. Except for the errorMessage at the end about invalid JSON

So i'd be in favor of changing the manifestparser explanation->warning for all items and errorMesssage on the toplevel. and also tweaking manifest-values.js to use these terms as well.

Copy link
Collaborator Author

@patrickhulce patrickhulce Aug 20, 2018

Choose a reason for hiding this comment

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

@brendankenny are you on board with warning as well? I think it reads rather well now (aside from the fact that it's warning = 'ERROR: 😆)

Copy link
Member

Choose a reason for hiding this comment

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

are you on board with warning as well?

yes, sounds good to me

(aside from the fact that it's warning = 'ERROR: 😆)

lol yes, that's terrible

let warning;

if (typeof raw === 'string') {
value = trim ? raw.trim() : raw;
} else {
if (raw !== undefined) {
debugString = 'ERROR: expected a string.';
warning = 'ERROR: expected a string.';
}
value = undefined;
}

return {
raw,
value,
debugString,
warning,
};
}

Expand All @@ -70,7 +70,7 @@ function parseColor(raw) {
const validatedColor = validateColor(color.raw);
if (!validatedColor) {
color.value = undefined;
color.debugString = 'ERROR: color parsing failed.';
color.warning = 'ERROR: color parsing failed.';
}

return color;
Expand Down Expand Up @@ -117,7 +117,7 @@ function parseStartUrl(jsonInput, manifestUrl, documentUrl) {
return {
raw,
value: documentUrl,
debugString: 'ERROR: start_url string empty',
warning: 'ERROR: start_url string empty',
};
}
const parsedAsString = parseString(raw);
Expand All @@ -135,7 +135,7 @@ function parseStartUrl(jsonInput, manifestUrl, documentUrl) {
return {
raw,
value: documentUrl,
debugString: 'ERROR: invalid start_url relative to ${manifestUrl}',
warning: 'ERROR: invalid start_url relative to ${manifestUrl}',
};
}

Expand All @@ -144,7 +144,7 @@ function parseStartUrl(jsonInput, manifestUrl, documentUrl) {
return {
raw,
value: documentUrl,
debugString: 'ERROR: start_url must be same-origin as document',
warning: 'ERROR: start_url must be same-origin as document',
};
}

Expand All @@ -165,7 +165,7 @@ function parseDisplay(jsonInput) {
return {
raw: jsonInput,
value: DEFAULT_DISPLAY_MODE,
debugString: parsedString.debugString,
warning: parsedString.warning,
};
}

Expand All @@ -174,15 +174,15 @@ function parseDisplay(jsonInput) {
return {
raw: jsonInput,
value: DEFAULT_DISPLAY_MODE,
debugString: 'ERROR: \'display\' has invalid value ' + displayValue +
warning: 'ERROR: \'display\' has invalid value ' + displayValue +
`. will fall back to ${DEFAULT_DISPLAY_MODE}.`,
};
}

return {
raw: jsonInput,
value: displayValue,
debugString: undefined,
warning: undefined,
};
}

Expand All @@ -195,7 +195,7 @@ function parseOrientation(jsonInput) {
if (orientation.value &&
!ALLOWED_ORIENTATION_VALUES.includes(orientation.value.toLowerCase())) {
orientation.value = undefined;
orientation.debugString = 'ERROR: \'orientation\' has an invalid value, will be ignored.';
orientation.warning = 'ERROR: \'orientation\' has an invalid value, will be ignored.';
}

return orientation;
Expand Down Expand Up @@ -223,13 +223,13 @@ function parseIcon(raw, manifestUrl) {
raw: raw.density,
value: 1,
/** @type {string|undefined} */
debugString: undefined,
warning: undefined,
};
if (density.raw !== undefined) {
density.value = parseFloat(density.raw);
if (isNaN(density.value) || !isFinite(density.value) || density.value <= 0) {
density.value = 1;
density.debugString = 'ERROR: icon density cannot be NaN, +∞, or less than or equal to +0.';
density.warning = 'ERROR: icon density cannot be NaN, +∞, or less than or equal to +0.';
}
}

Expand All @@ -242,7 +242,7 @@ function parseIcon(raw, manifestUrl) {
sizes = {
raw: raw.sizes,
value: set.size > 0 ? Array.from(set) : undefined,
debugString: undefined,
warning: undefined,
};
} else {
sizes = {...parsedSizes, value: undefined};
Expand All @@ -256,7 +256,7 @@ function parseIcon(raw, manifestUrl) {
density,
sizes,
},
debugString: undefined,
warning: undefined,
};
}

Expand All @@ -272,7 +272,7 @@ function parseIcons(jsonInput, manifestUrl) {
raw,
/** @type {Array<ReturnType<typeof parseIcon>>} */
value: [],
debugString: undefined,
warning: undefined,
};
}

Expand All @@ -281,7 +281,7 @@ function parseIcons(jsonInput, manifestUrl) {
raw,
/** @type {Array<ReturnType<typeof parseIcon>>} */
value: [],
debugString: 'ERROR: \'icons\' expected to be an array but is not.',
warning: 'ERROR: \'icons\' expected to be an array but is not.',
};
}

Expand All @@ -298,7 +298,7 @@ function parseIcons(jsonInput, manifestUrl) {
return {
raw,
value,
debugString: undefined,
warning: undefined,
};
}

Expand All @@ -317,7 +317,7 @@ function parseApplication(raw) {
appUrl.value = new URL(appUrl.value).href;
} catch (e) {
appUrl.value = undefined;
appUrl.debugString = 'ERROR: invalid application URL ${raw.url}';
appUrl.warning = 'ERROR: invalid application URL ${raw.url}';
}
}

Expand All @@ -328,7 +328,7 @@ function parseApplication(raw) {
id,
url: appUrl,
},
debugString: undefined,
warning: undefined,
};
}

Expand All @@ -342,15 +342,15 @@ function parseRelatedApplications(jsonInput) {
return {
raw,
value: undefined,
debugString: undefined,
warning: undefined,
};
}

if (!Array.isArray(raw)) {
return {
raw,
value: undefined,
debugString: 'ERROR: \'related_applications\' expected to be an array but is not.',
warning: 'ERROR: \'related_applications\' expected to be an array but is not.',
};
}

Expand All @@ -364,7 +364,7 @@ function parseRelatedApplications(jsonInput) {
return {
raw,
value,
debugString: undefined,
warning: undefined,
};
}

Expand All @@ -374,21 +374,21 @@ function parseRelatedApplications(jsonInput) {
function parsePreferRelatedApplications(jsonInput) {
const raw = jsonInput.prefer_related_applications;
let value;
let debugString;
let warning;

if (typeof raw === 'boolean') {
value = raw;
} else {
if (raw !== undefined) {
debugString = 'ERROR: \'prefer_related_applications\' expected to be a boolean.';
warning = 'ERROR: \'prefer_related_applications\' expected to be a boolean.';
}
value = undefined;
}

return {
raw,
value,
debugString,
warning,
};
}

Expand Down Expand Up @@ -425,7 +425,7 @@ function parse(string, manifestUrl, documentUrl) {
return {
raw: string,
value: undefined,
debugString: 'ERROR: file isn\'t valid JSON: ' + e,
warning: 'ERROR: file isn\'t valid JSON: ' + e,
Copy link
Member

@brendankenny brendankenny Aug 20, 2018

Choose a reason for hiding this comment

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

I think @paulirish wanted this to be errorMessage, as this was the one true place it can fail (everywhere else it falls back to a default)

Copy link
Member

Choose a reason for hiding this comment

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

(it will be interesting to see if you change it just here (and errorMessage: undefined below) if tsc will correctly pick up all the places it needs to be adjusted automatically since the artifact is dynamically typed from the inferred return value of this function...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh meant to reply to this, this was an intentional feigned ignorance about that part of the comment ;)

I don't think manifest-parser should to try to name its properties based on how one audit will pass/fail because of its value. From the perspective of manifest-parser all of these strings are messages explaining that something was abnormal about the raw value (there are other cases that we ignore the string and provide a default where it's a warning).

Since we're taking the stance that these are all warnings and not total failures, seems like we should lean in to it :)

Copy link
Member

Choose a reason for hiding this comment

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

on the one hand.. this field represents why the manifest parser was unable to parse.
but on the other hand.. i dont really care. 😑 i'm fine with warning if you'd prefer it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol, I'd prefer it given the agreement we have around everything else in there labelled ERROR: being a warning :)

sg!

Copy link
Member

@brendankenny brendankenny Aug 22, 2018

Choose a reason for hiding this comment

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

this field represents why the manifest parser was unable to parse.

I also agree with this. It's the only real failure for a manifest. Everything else is a warning that something was malformed but the browser will fall back to a default, so the manifest will still work.

But I also don't care that much :) it's just (minor) ergonomics of the result, not correctness.

};
}

Expand All @@ -447,7 +447,7 @@ function parse(string, manifestUrl, documentUrl) {
return {
raw: string,
value: manifest,
debugString: undefined,
warning: undefined,
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/audits/webapp-install-banner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ describe('PWA: webapp install banner audit', () => {

it('includes warning from start_url', () => {
const artifacts = generateMockArtifacts();
artifacts.StartUrl = {statusCode: 200, debugString: 'Warning!'};
artifacts.StartUrl = {statusCode: 200, explanation: 'Warning!'};

return WebappInstallBannerAudit.audit(artifacts).then(result => {
assert.strictEqual(result.rawValue, true);
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/gather/gatherers/manifest-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ describe('Manifest gatherer', () => {
url: EXAMPLE_DOC_URL,
}).then(artifact => {
assert.ok(typeof artifact.value === 'object');
assert.strictEqual(artifact.debugString, undefined);
assert.strictEqual(artifact.explanation, undefined);
});
});
});
Loading