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

patrickhulce · 2018-12-28T18:39:32Z

Summary
We've talked about being able to reuse prior artifacts in other future gatherers but were always afraid of the precedence it sets for some horrifying graph of dependencies. This really is great use case though, so I wanted to play with it and see how it feels.

Therefore, I named the method dangerouslyUsePreviousArtifact in the hopes it assuages some concerns about this starting to be used everywhere 😆

Waiting to update start-url test depending on what kind of reaction this gets :)

Related Issues/PRs
fixes #6881

paulirish

it's crazy but i'm into it. 👍

paulirish · 2019-01-04T01:11:42Z

lighthouse-core/gather/gather-runner.js

@@ -458,6 +469,16 @@ class GatherRunner {
          passConfig,
          // *pass() functions and gatherers can push to this warnings array.
          LighthouseRunWarnings: baseArtifacts.LighthouseRunWarnings,
+          /** @param {keyof LH.GathererArtifacts} name */
+          async dangerouslyUsePreviousArtifact(name) {


feels like this fn should be defined outside of this for loop.

what would be your preferred flavor?

dangerouslyUsePreviousArtifact: GatherRunner.dangerouslyUsePreviousArtifact.bind(gathererResults)
dangerouslyUsePreviousArtifact: GatherRunner.createPreviousArtifactFn(gathererResults)
dangerouslyUsePreviousArtifact(name) { return GatherRunner.dangerouslyUsePreviousArtifact(gathererResults, name); }

dangerouslyUsePreviousArtifact: GatherRunner.dangerouslyUsePreviousArtifact.bind(gathererResults)

brendankenny

ha, probably to no one's surprise, I'm not super into this :) but I think it's definitely worth thinking about.

We've had use cases before but never a super necessary one (they always end up "or we could do it this somewhat less convenient way"), but maybe this is it.

patrickhulce · 2019-01-04T15:47:38Z

We've had use cases before but never a super necessary one, but maybe this is it.

I do think #6881 presents the super necessary use case :D

brendankenny · 2019-01-04T20:30:18Z

We've had use cases before but never a super necessary one, but maybe this is it.

I do think #6881 presents the super necessary use case :D

ha, sorry, I meant to say super necessary to share artifacts between gatherers instead of finding another solution. We definitely definitely should fix #6881 :) :)

paulirish · 2019-01-07T20:50:03Z

DZL, do a barrel roll!

devtools-bot · 2019-01-07T21:03:48Z

DZL is done! Go check it out http://lh-dzl-6882.surge.sh

patrickhulce · 2019-01-08T02:57:25Z

@brendankenny what concretely does

I'm not super into this :) but I think it's definitely worth thinking about.

mean :)

Is it any of the following....?

I won't approve a PR that does this, but it's a starting point for a discussion on what to do.
I might approve a PR that does this, but I need 24 hours to think about it.
I would approve a PR that does this after I review it.

brendankenny · 2019-01-08T21:33:01Z

Is it any of the following....?

My hesitation is that we already have a lot of implicit interactions in the gathering stage (online/offline, affects perf while a trace is being collected, etc), but at least the RequiredArtifacts system works well for figuring out the dependency graph when filtering audits and etc. Adding this method breaks that and it can only stay kind of broken or get worse from here :)

A couple of other options:

fix upstream (we didn't really look into what change caused Lighthouse unable to fetch manifest.json, favicons in Canary 73.0.3654.0 #6881. Something in Chrome's handling of manifests? Or a protocol change in relation to offline?)
work around (if the change was for a good reason, maybe there's a better way to do it)
treat manifests as special like Chrome does. Chrome fetches the manifest on the first visit to a site and has it available from a cache on future visits, even offline. We could do the same thing in LH, adding it to baseArtifacts when it's fetched, like finalUrl is updated.

I recognize the underlying issue that it's still vaguely reasonable to want to access artifacts between gatherers, but I think I favor the third option, since it lets us avoid that complexity and it matches how Chrome treats manifests as part of the meta information available for a site. I think we also have some help here, as I recently read that Chrome always fetches the manifest on first visit if a site has one, so we actually don't even need the driver.getAppManifest() call to get it out of the network traffic.

patrickhulce · 2019-01-08T22:48:53Z

We could do the same thing in LH, adding it to baseArtifacts when it's fetched, like finalUrl is updated.

I can get behind this 👍

patrickhulce · 2019-01-08T23:31:19Z

btw @brendankenny I skipped the first two because ~~the bug has already been fixed a bit ago in Chrome :)~~ Maybe not, see updated issue this is really just about making our starturl gatherer not refetch/reparse (and clumsily so it's currently susceptible to BOM issues for example)

I'll update the issue with a comment.

brendankenny · 2019-01-08T23:37:54Z

btw @brendankenny I skipped the first two because the bug has already been fixed a bit ago in Chrome :)

oh, haha, whoops. I had no idea :)

patrickhulce · 2019-01-09T05:43:12Z

@brendankenny scratch that last thing I said, the author fixed the service worker and I attributed that to a 1-day Canary bug instead 😆 they will be sharing source code though so we can also get to bottom of it

brendankenny · 2019-01-10T02:17:39Z

closing in favor of #6957

patrickhulce added 2 commits December 28, 2018 12:35

core(starturl): re-use already fetched manifest

8e65fac

better error messages

95a7b72

patrickhulce requested a review from paulirish as a code owner December 28, 2018 18:39

lint

85701d4

paulirish reviewed Jan 4, 2019

View reviewed changes

brendankenny reviewed Jan 4, 2019

View reviewed changes

patrickhulce mentioned this pull request Jan 8, 2019

core: special case AppManifest as baseArtifact #6957

Merged

brendankenny closed this Jan 10, 2019

brendankenny deleted the reuse_manifest branch March 20, 2019 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

patrickhulce commented Dec 28, 2018 •

edited

Loading

paulirish left a comment

paulirish Jan 4, 2019

patrickhulce Jan 4, 2019

paulirish Jan 4, 2019

brendankenny left a comment

patrickhulce commented Jan 4, 2019

brendankenny commented Jan 4, 2019

paulirish commented Jan 7, 2019

devtools-bot commented Jan 7, 2019

patrickhulce commented Jan 8, 2019

brendankenny commented Jan 8, 2019

patrickhulce commented Jan 8, 2019

patrickhulce commented Jan 8, 2019 •

edited

Loading

brendankenny commented Jan 8, 2019

patrickhulce commented Jan 9, 2019

brendankenny commented Jan 10, 2019

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

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

Conversation

patrickhulce commented Dec 28, 2018 • edited Loading

paulirish left a comment

Choose a reason for hiding this comment

paulirish Jan 4, 2019

Choose a reason for hiding this comment

patrickhulce Jan 4, 2019

Choose a reason for hiding this comment

paulirish Jan 4, 2019

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented Jan 4, 2019

brendankenny commented Jan 4, 2019

paulirish commented Jan 7, 2019

devtools-bot commented Jan 7, 2019

patrickhulce commented Jan 8, 2019

brendankenny commented Jan 8, 2019

patrickhulce commented Jan 8, 2019

patrickhulce commented Jan 8, 2019 • edited Loading

brendankenny commented Jan 8, 2019

patrickhulce commented Jan 9, 2019

brendankenny commented Jan 10, 2019

patrickhulce commented Dec 28, 2018 •

edited

Loading

patrickhulce commented Jan 8, 2019 •

edited

Loading