core: special case AppManifest as baseArtifact #6957

patrickhulce · 2019-01-08T23:33:04Z

Cleans up our start URL gatherer to not fetch and parse the manifest again.

Related Issues/PRs
@brendankenny's preferred implementation of #6882
closes #6881

brendankenny

I like this :)

brendankenny · 2019-01-09T20:07:23Z

types/artifacts.d.ts

@@ -29,6 +29,8 @@ declare global {
      NetworkUserAgent: string;
      /** The benchmark index that indicates rough device class. */
      BenchmarkIndex: number;
+      /** The application manifest if one was fetched. */
+      AppManifest: Artifacts.Manifest | null;


to me this makes me question if it's actually related to an appcache manifest :) If we want to be more precise, how do you feel about WebAppManifest?

someone should tell driver and the devtools protocol then 😛

that wfm 👍

brendankenny · 2019-01-09T20:08:40Z

lighthouse-core/gather/gather-runner.js

+   * @param {LH.Gatherer.PassContext} passContext
+   * @return {Promise<LH.Artifacts['Manifest']>}
+   */
+  static async getAppManifest(passContext) {


should we merge most of this into driver.getAppManifest()? Not the baseArtifacts check, of course, and probably not the parse step(?)

yeah good idea 👍

brendankenny · 2019-01-09T20:10:18Z

lighthouse-core/gather/gather-runner.js

+
+    const manifestPromise = passContext.driver.getAppManifest();
+    /** @type {Promise<void>} */
+    const timeoutPromise = new Promise(resolve => setTimeout(resolve, 3000));


I think we can remove this with the default timeouts now

I like the idea of keeping it short though, I'll set it to 3s 👍

brendankenny · 2019-01-09T20:12:22Z

lighthouse-core/gather/gatherers/manifest.js

- * Uses the debugger protocol to fetch the manifest from within the context of
- * the target page, reusing any credentials, emulation, etc, already established
- * there. The artifact produced is the fetched string, if any, passed through
+ * The artifact produced is the fetched manifest string, if any, passed through
 * the manifest parser.
 */
 class Manifest extends Gatherer {


any reason to keep this? TODO for followup to split up the changes nicely? Or is it worth keeping for some reason?

meh, I'll just do it now

boy am I regretting that 😆

yeah I'm gonna make this a TODO

brendankenny · 2019-01-09T20:16:10Z

types/gatherer.d.ts

@@ -20,6 +20,7 @@ declare global {
      options?: object;
      /** Push to this array to add top-level warnings to the LHR. */
      LighthouseRunWarnings: Array<string>;
+      baseArtifacts: BaseArtifacts;


we might want to play around with Readonly for this, but that can be something for another day :)

brendankenny · 2019-01-09T20:20:27Z

lighthouse-core/gather/gatherers/start-url.js

@@ -61,7 +54,7 @@ class StartUrl extends Gatherer {
   * @param {string} startUrl
   * @return {Promise<{statusCode: number, explanation: string}>}
   */
-  _attemptManifestFetch(driver, startUrl) {
+  _attemptStartURLFetch(driver, startUrl) {


brendankenny · 2019-01-09T20:25:07Z

looks like some fragile mocks broke :S

patrickhulce · 2019-01-16T17:26:32Z

@brendankenny this is ready for a second look now btw

brendankenny · 2019-01-17T00:39:50Z

types/artifacts.d.ts

@@ -29,6 +29,8 @@ declare global {
      NetworkUserAgent: string;
      /** The benchmark index that indicates rough device class. */
      BenchmarkIndex: number;
+      /** Parsed version of the page's Web App Manifest, or null if none found.. */


no double .

brendankenny · 2019-01-17T00:52:39Z

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

+
+  describe('.getAppManifest', () => {
+    it('should return null when no manifest', async () => {
+      const sendCommand = jest.spyOn(connection, 'sendCommand');


spyOn is a little hard to read to me because I expect it to be spied upon at that point. How do you feel about just doing the plain connection.sendCommand = jest.fn().mockResolvedValueOnce({data: undefined, url: '/manifest'});?

sure that's fine

brendankenny · 2019-01-17T00:58:19Z

lighthouse-core/gather/gather-runner.js

+   * will have the reason. See manifest-parser.js for more information.
+   *
+   * @param {LH.Gatherer.PassContext} passContext
+   * @return {Promise<LH.Artifacts['Manifest']>}


since we're going to eventually ditch LH.Artifacts['Manifest'], switch to LH.Artifacts['WebAppManifest'] (or LH.Artifacts.Manifest|null)?

brendankenny · 2019-01-17T01:01:07Z

lighthouse-core/gather/gather-runner.js

@@ -467,6 +493,7 @@ class GatherRunner {
        }
        await GatherRunner.beforePass(passContext, gathererResults);
        await GatherRunner.pass(passContext, gathererResults);
+        baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext);


we have two other checks against isFirstPass already in this method, can we move the check for this line out here too and avoid putting it on passContext (and it'll be more obvious we aren't overwriting it on every turn of the loop)?

brendankenny · 2019-01-17T01:06:26Z

lighthouse-core/gather/gatherers/start-url.js

  }

  /**
   * Read the parsed manifest and return failure reasons or the startUrl
-   * @param {?{value?: {start_url: {value: string, warning?: string}}, warning?: string}} manifest
+   * @param {LH.Artifacts['Manifest']} manifest


LH.Artifacts['WebAppManifest']

brendankenny · 2019-01-17T01:10:47Z

lighthouse-core/test/gather/gatherers/start-url-test.js

-        assert.equal(artifact.explanation, 'Timed out waiting for fetched start_url.');
-      });
+    const resultPromise = startUrlGatherer.afterPass(passContext);
+    jest.advanceTimersByTime(5000);


wellllllll this is nice

brendankenny · 2019-01-17T01:12:23Z

lighthouse-core/test/gather/gatherers/start-url-test.js

      driver: wrapSendCommand(mockDriver, 'https://ifixit-pwa.appspot.com/', 200, true),
    };
-    const optionsWithQueryString = {
+    const passContextWithFragment = {


brendankenny · 2019-01-17T01:13:06Z

lighthouse-core/test/gather/gatherers/start-url-test.js

+  });
+
+  afterEach(() => {
+    jest.advanceTimersByTime(5000);


why is this needed after each test is done?

the other tests not touched by this don't properly cleanup their timer, I'll add a todo to fix this another time?

the other tests not touched by this don't properly cleanup their timer, I'll add a todo to fix this another time?

yeah, sounds good. I think advanceTimersByTime is going to be a game changer for a lot of our older tests and it would be nice to update anyways

brendankenny · 2019-01-17T01:14:00Z

lighthouse-core/gather/gather-runner.js

+   */
+  static async getWebAppManifest(passContext) {
+    // If we already have a manifest, return it.
+    if (passContext.baseArtifacts.WebAppManifest) return passContext.baseArtifacts.WebAppManifest;


not needed with the isFirstPass check below?

fair, removed

paulirish

a lot cleaner to my eyes. excited for it.

brendankenny

yeah! love it

brendankenny · 2019-01-17T20:49:19Z

lighthouse-core/test/gather/gatherers/start-url-test.js

+  });
+
+  afterEach(() => {
+    jest.advanceTimersByTime(5000);


the other tests not touched by this don't properly cleanup their timer, I'll add a todo to fix this another time?

yeah, sounds good. I think advanceTimersByTime is going to be a game changer for a lot of our older tests and it would be nice to update anyways

tmb-github · 2019-01-28T19:43:58Z

Whatever's been done with the PWA tester in Lighthouse seems to have (or may have) broken it with regards to testing Performance in the other parts of the audit, as well as blocked the PWA audit score as well.

In Chrome 71 (still current as I write this), this site gets 100% on performance and PWA; in the Chrome 74.0.3686.0 available today, this site gets 0% performance and 0% PWA despite 100 in all PWA categories except 2 of them:

https://ives-fourth-symphony.com

This may or may not be related to the manifest fetching rewrite, but the coincidence of the two prompted me to report this here.

patrickhulce · 2019-01-28T20:08:27Z

Thanks @tmb-github! The version of Lighthouse in Chrome doesn't have this commit yet, and locally this hash passes on that site, so looks like they're unrelated.

Is it possible you're bumping into #6968?

tmb-github · 2019-01-29T21:17:47Z

Please try the same audit again on the new stable Chrome 72.0.3626.81 (Official Build) (64-bit), released today. Nothing can be fetched for the PWA audit nor the Performance audit.

tmb-github · 2019-01-29T21:31:52Z

patrickhulce · 2019-01-29T21:32:52Z

Thanks @tmb-github it's due to some infrastructure serving outdated version of audits panel that's incompatible with newer Chrome. The team is on it.

core: special case AppManifest as baseArtifact

d36304d

patrickhulce requested a review from paulirish as a code owner January 8, 2019 23:33

brendankenny reviewed Jan 9, 2019

View reviewed changes

just feedback

6073f99

brendankenny mentioned this pull request Jan 10, 2019

core(starturl): re-use already fetched manifest #6882

Closed

patrickhulce added 3 commits January 9, 2019 21:01

fix tests

09869b0

todo

633829e

cleanup start url

5cca8a9

brendankenny reviewed Jan 17, 2019

View reviewed changes

patrickhulce added 3 commits January 16, 2019 20:34

feedback

78ba22d

Merge branch 'master' into manifest_base_artifacts

213bd36

merge fixes

bc6dfd5

paulirish reviewed Jan 17, 2019

View reviewed changes

paulirish assigned brendankenny Jan 17, 2019

brendankenny approved these changes Jan 17, 2019

View reviewed changes

brendankenny merged commit fb6c376 into master Jan 17, 2019

brendankenny deleted the manifest_base_artifacts branch January 17, 2019 20:53

patrickhulce mentioned this pull request Jan 23, 2019

core(manifest): remove Manifest artifact #7078

Merged

patrickhulce mentioned this pull request Jul 24, 2019

core(start-url): stay offline for entirety of offlinePass #9451

Merged

core: special case AppManifest as baseArtifact #6957

core: special case AppManifest as baseArtifact #6957

Conversation

patrickhulce commented Jan 8, 2019

brendankenny 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 9, 2019

patrickhulce commented Jan 16, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmb-github commented Jan 28, 2019

patrickhulce commented Jan 28, 2019

tmb-github commented Jan 29, 2019

tmb-github commented Jan 29, 2019

patrickhulce commented Jan 29, 2019