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.

Sign up for GitHub

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

Jump to bottom

core(installable-manifest): expose app manifest url #11330

Merged
merged 5 commits into from Sep 1, 2020
Merged

core(installable-manifest): expose app manifest url #11330

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8232811
modified manifest artifact, added test, exposing url in audit
adrianaixba Aug 26, 2020
7d35259
reverting things on gather-runner
adrianaixba Aug 28, 2020
aff9f37
added smoke test pwa2 expectation
adrianaixba Aug 28, 2020
1323a39
manifestUrl
adrianaixba Aug 28, 2020
fb31832
sample_v2 fix
adrianaixba Aug 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion lighthouse-core/audits/installable-manifest.js
View file
Open in desktop
Expand Up @@ -89,17 +89,20 @@ class InstallableManifest extends MultiCheckAudit {
/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<{failures: Array<string>, manifestValues: LH.Artifacts.ManifestValues}>}
* @return {Promise<{failures: Array<string>, manifestValues: LH.Artifacts.ManifestValues, manifestUrl: string | undefined}>}
*/
static async audit_(artifacts, context) {
const manifestValues = await ManifestValues.request(artifacts, context);
const manifestFailures = InstallableManifest.assessManifest(manifestValues);

const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.manifestUrl : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

connor will know better but i think we want null rather than undefined

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea null is preferable for "missing", especially when the object may be JSON serialized (properties set to undefined are dropped in JSON)


return {
Copy link
Member

Choose a reason for hiding this comment

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

side note how do we have an audit that doesn't have a score.. so confused

Copy link
Collaborator

Choose a reason for hiding this comment

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

extends MultiCheckAudit and that audit_ life, def confusing though

Copy link
Member

Choose a reason for hiding this comment

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

OHHHH MultiCheckAudit. of course.

failures: [
...manifestFailures,
],
manifestValues,
manifestUrl,
};
}
}
Expand Down
11 changes: 8 additions & 3 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Expand Up @@ -584,7 +584,9 @@ class GatherRunner {
}

// Fetch the manifest, if it exists.
baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext);
const manifestResult = await GatherRunner.getWebAppManifest(passContext);
// manifestResult could be null
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we follow this comment style guide:

// Start with capital letter. End with punctuation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(oh, but it does seem like we can revert these few lines?)

baseArtifacts.WebAppManifest = manifestResult ? manifestResult.parsedManifest : null;

if (baseArtifacts.WebAppManifest) {
baseArtifacts.InstallabilityErrors = await GatherRunner.getInstallabilityErrors(passContext);
Expand Down Expand Up @@ -626,12 +628,15 @@ class GatherRunner {
* will have the reason. See manifest-parser.js for more information.
*
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts.Manifest|null>}
* @return {Promise<{parsedManifest: LH.Artifacts.Manifest|null, manifestUrl: string|null} | null>}
*/
static async getWebAppManifest(passContext) {
const response = await passContext.driver.getAppManifest();
if (!response) return null;
return manifestParser(response.data, response.url, passContext.url);
const parsedManifest = manifestParser(response.data, response.url, passContext.url);
const manifestUrl = response.url;
Copy link
Member

Choose a reason for hiding this comment

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

we talked in voice about this.. should manifestUrl be part of the parser's return value? I said no, but... let's reconsider. ... it could. and it kinda makes sense. just spoke with @brendankenny and we both feel fairly ambivalent about it.

It keeps things a tad cleaner to put it in the manifestParser return, so let's do that. (well, as it turns out... you already did)

that actually means you can revert allllll the changes in gather-runner

anddddddddddd i think everything will just still end up working fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea it seems like this whole file could be reverted and the url from the parser lib will be consumable in installable-manifest

const manifestResult = {parsedManifest, manifestUrl};
return manifestResult;
}

/**
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/lib/manifest-parser.js
View file
Open in desktop
Expand Up @@ -505,6 +505,7 @@ function parse(string, manifestUrl, documentUrl) {
raw: string,
value: manifest,
warning: manifestUrlWarning,
manifestUrl: manifestUrl,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think url would be a better name here, considering the natural way to name a variable holding this object is manifest (gather-runner actually uses parsedManifest which is also cool), and manifest.manifestUrl is redundant.

I think manifest.url would not lead to any confusion about what the url is referring to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think we should do this for the audit items value too? details.items[0].url vs details.items[0].manifestUrl. manifestUrl does add some clarity, although not sure if its necessary given that the audit refers to the manifest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

url would be great there too

Copy link
Member

Choose a reason for hiding this comment

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

There is some pretty good self documenting value for keeping it manifestUrl when it's deep in installableManifest.details.items[0], though, since we don't publish docs for specific audit's details.

Note also it's a term from the spec, to differentiate from "document URL" and start_url (which are both also needed for parsing the manifest).

};
}

Expand Down
10 changes: 10 additions & 0 deletions lighthouse-core/test/audits/installable-manifest-test.js
View file
Open in desktop
Expand Up @@ -44,6 +44,16 @@ describe('PWA: webapp install banner audit', () => {
});
});

it('passes when manifestUrl matches', () => {
const artifacts = generateMockArtifacts();
const context = generateMockAuditContext();

return InstallableManifestAudit.audit(artifacts, context).then(result => {
assert.strictEqual(artifacts.WebAppManifest.manifestUrl, EXAMPLE_MANIFEST_URL);
assert.strictEqual(result.details.items[0].manifestUrl, EXAMPLE_MANIFEST_URL);
});
});

it('fails with a non-parsable manifest', () => {
const artifacts = generateMockArtifacts('{,:}');
const context = generateMockAuditContext();
Expand Down
5 changes: 3 additions & 2 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Expand Up @@ -1798,8 +1798,9 @@ describe('GatherRunner', function() {
.mockResponse('Page.getInstallabilityErrors', {installabilityErrors: []});

const result = await GatherRunner.getWebAppManifest(passContext);
expect(result).toHaveProperty('raw', JSON.stringify(manifest));
expect(result && result.value).toMatchObject({
const parsedManifest = result ? result.parsedManifest : null;
expect(parsedManifest).toHaveProperty('raw', JSON.stringify(manifest));
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be reverted to after you revert the changes in gather-runner, but I think an assertion here for result.url would be good.

expect(parsedManifest && parsedManifest.value).toMatchObject({
name: {value: 'App', raw: 'App'},
start_url: {value: passContext.url, raw: undefined},
});
Expand Down