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 1 commit
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.explanation
Copy link
Member

Choose a reason for hiding this comment

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

warning/errorMessage?

* 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, explanation?: string}}, explanation?: 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.explanation;

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 explanation here and continue with the fetch.
if (manifest.value.start_url.explanation) {
return {isReadFailure: true, reason: manifest.value.start_url.explanation};
}

// @ts-ignore - TODO(bckenny): should actually be testing value above, not debugString
// @ts-ignore - TODO(bckenny): should actually be testing value above, not explanation
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 explanation;

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

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

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.explanation = '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',
explanation: '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}',
explanation: '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',
explanation: '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,
explanation: parsedString.explanation,
};
}

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

return {
raw: jsonInput,
value: displayValue,
debugString: undefined,
explanation: 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.explanation = '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,
explanation: 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.explanation = '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,
explanation: undefined,
};
} else {
sizes = {...parsedSizes, value: undefined};
Expand All @@ -256,7 +256,7 @@ function parseIcon(raw, manifestUrl) {
density,
sizes,
},
debugString: undefined,
explanation: undefined,
};
}

Expand All @@ -272,7 +272,7 @@ function parseIcons(jsonInput, manifestUrl) {
raw,
/** @type {Array<ReturnType<typeof parseIcon>>} */
value: [],
debugString: undefined,
explanation: 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.',
explanation: '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,
explanation: 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.explanation = 'ERROR: invalid application URL ${raw.url}';
}
}

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

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

if (!Array.isArray(raw)) {
return {
raw,
value: undefined,
debugString: 'ERROR: \'related_applications\' expected to be an array but is not.',
explanation: '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,
explanation: undefined,
};
}

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

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

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

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,
explanation: 'ERROR: file isn\'t valid JSON: ' + e,
};
}

Expand All @@ -447,7 +447,7 @@ function parse(string, manifestUrl, documentUrl) {
return {
raw: string,
value: manifest,
debugString: undefined,
explanation: 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