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

adrianaixba · 2020-08-26T22:44:23Z

exposes the app manifest url in the installable manifest audit

paulirish · 2020-08-27T19:41:13Z

lighthouse-core/audits/installable-manifest.js

   */
  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;


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

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

paulirish · 2020-08-27T21:07:49Z

lighthouse-core/gather/gather-runner.js

   */
  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;


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.

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

paulirish · 2020-08-27T21:10:02Z

lighthouse-core/audits/installable-manifest.js

   */
  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;
+
    return {


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

extends MultiCheckAudit and that audit_ life, def confusing though

OHHHH MultiCheckAudit. of course.

connorjclark

+1 to Paul's comment, it seems you two worked out that just changing the return value of the manifest parser is enough to get installable-manifest the url. No need for any of the gather-runner changes afaict

An end-to-end to would be good here, can you add to the expectations file for one of our smoke tests? pwa1 and pwa2 both load pages that have a manifest, so you could add an assertion there for installable-manifest.

connorjclark · 2020-08-27T22:31:27Z

lighthouse-core/lib/manifest-parser.js

@@ -505,6 +505,7 @@ function parse(string, manifestUrl, documentUrl) {
    raw: string,
    value: manifest,
    warning: manifestUrlWarning,
+    manifestUrl: manifestUrl,


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.

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.

url would be great there too

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).

connorjclark · 2020-08-27T22:32:35Z

lighthouse-core/gather/gather-runner.js

@@ -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


nit: we follow this comment style guide:

// Start with capital letter. End with punctuation.

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

connorjclark · 2020-08-27T22:35:47Z

lighthouse-core/gather/gather-runner.js

   */
  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;


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

connorjclark · 2020-08-27T22:37:59Z

lighthouse-core/audits/installable-manifest.js

   */
  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;


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

connorjclark · 2020-08-27T22:39:51Z

lighthouse-core/test/gather/gather-runner-test.js

-      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));


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.

connorjclark

LGTM, nice job!

connorjclark · 2020-09-01T00:38:20Z

fyi if you put "fixed by #blah" in the first post, when the PR merges it auto-closes the associated issue

modified manifest artifact, added test, exposing url in audit

8232811

adrianaixba requested a review from a team as a code owner August 26, 2020 22:44

adrianaixba requested review from connorjclark and removed request for a team August 26, 2020 22:44

googlebot added the cla: yes label Aug 26, 2020

devtools-bot assigned connorjclark Aug 26, 2020

devtools-bot added the waiting4reviewer label Aug 26, 2020

paulirish reviewed Aug 27, 2020

View reviewed changes

connorjclark requested changes Aug 27, 2020

View reviewed changes

connorjclark added waiting4committer and removed waiting4reviewer labels Aug 27, 2020

reverting things on gather-runner

7d35259

vercel bot deployed to Preview August 28, 2020 19:18 View deployment

adrianaixba added 2 commits August 28, 2020 15:34

added smoke test pwa2 expectation

aff9f37

manifestUrl

1323a39

vercel bot deployed to Preview August 28, 2020 20:55 View deployment

sample_v2 fix

fb31832

vercel bot deployed to Preview August 28, 2020 21:23 View deployment

adrianaixba added waiting4reviewer and removed waiting4committer labels Aug 28, 2020

connorjclark changed the title ~~core: expose app manifest url on installable manifest audit~~ core(installable-manifest): expose app manifest url Aug 28, 2020

connorjclark approved these changes Sep 1, 2020

View reviewed changes

connorjclark merged commit 9ba2cd4 into master Sep 1, 2020

connorjclark deleted the app-manifest-urls branch September 1, 2020 00:37

connorjclark mentioned this pull request Sep 1, 2020

Surface manifest and service worker URLs in JSON results #9683

Closed

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Sep 1, 2020

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

349fe91

adrianaixba removed the waiting4reviewer label Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adrianaixba commented Aug 26, 2020

paulirish Aug 27, 2020

connorjclark Aug 27, 2020

paulirish Aug 27, 2020

connorjclark Aug 27, 2020

paulirish Aug 27, 2020

patrickhulce Aug 27, 2020

paulirish Aug 27, 2020

connorjclark left a comment

connorjclark Aug 27, 2020

adrianaixba Aug 28, 2020

connorjclark Aug 28, 2020

brendankenny Aug 28, 2020 •

edited

Loading

connorjclark Aug 27, 2020

connorjclark Aug 27, 2020

connorjclark Aug 27, 2020

connorjclark Aug 27, 2020

connorjclark Aug 27, 2020

connorjclark left a comment

connorjclark commented Sep 1, 2020

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

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

Conversation

adrianaixba commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment

Choose a reason for hiding this comment

connorjclark commented Sep 1, 2020

brendankenny Aug 28, 2020 •

edited

Loading