core(fr): convert main-document-content gatherer #12470

adamraine · 2021-05-12T16:50:01Z

adamraine · 2021-05-12T16:50:35Z

lighthouse-core/lib/network-request.js

+   * @param {number} [timeout]
+   * @return {Promise<string>}
+   */
+  static async getRequestContent(session, requestId, timeout = 1000) {


This felt like the most convenient location for this function. I'm open to other suggestions though.

This was my first thought as well, but it does feel a bit weird to introduce session into this file.

My other thought was on NetworkMonitor. How does that feel to you?

I think I could go either way.

It also feels weird to create a new NetworkMonitor(session) just to use this function with session. I think I prefer leaving as is.

if it's a gathering helper method it seems like it should live under driver/, but it seems like it should just go in a new file if none of the existing helper modules are a good home for it? If the stricter timeout is important so we want people getting response content this way, it should preferably not be tucked in somewhere that's already relatively large and has an important separate purpose or people are just going to call Network.getResponseBody directly because they won't know it's there

actually, looking at the original PR (#4718), the stricter Network.getResponseBody timeout was specifically for working around using the protocol via the extension chrome.debugger API and there wasn't an issue in devtools/CLI. The extension doesn't use the protocol anymore, so should we just get rid of the getRequestContent method and everyone can call sendCommand('Network.getResponseBody', {requestId}) directly?

it should preferably not be tucked in somewhere that's already relatively large and has an important separate purpose or people are just going to call Network.getResponseBody directly because they won't know it's there

This was my concern with mingling session work in this file as well. You've boosted me beyond "could go either way" to "should probably stay in driver/ somewhere" :)

so should we just get rid of the getRequestContent method and everyone can call sendCommand('Network.getResponseBody', {requestId}) directly?

I would lean no. Defaulting this to a low timeout is a good thing, and the detail with NetworkRequest.getRequestIdForBackend is easy to forget, so bundling those together at least is necessary (otherwise your exact same argument of missing an argument tucked away somewhere is likely to happen here).

ah yeah, I missed NetworkRequest.getRequestIdForBackend in there.

The timeout still seems like cruft (should the comment point to this discussion now since #4718 can't happen anymore? :P) but it's pre-existing so 🤷

It also feels weird to create a new NetworkMonitor(session) just to use this function with session. I think I prefer leaving as is.

I was thinking just a static method like it is here. The alternative I see is a solo file.

I have moved it to a new file, network.js

adamraine · 2021-05-12T16:51:17Z

tsconfig.json

@@ -52,6 +52,7 @@
    "lighthouse-core/test/gather/gatherers/http-redirect-test.js",
    "lighthouse-core/test/gather/gatherers/js-usage-test.js",
    "lighthouse-core/test/gather/gatherers/link-elements-test.js",
+    "lighthouse-core/test/gather/gatherers/main-document-content-test.js",


Type checking would be a pain, and we probably won't need this test once afterPass is deprecated.

nooooo, but you've been so great about all the tests so far, and this is a nice opportunity to expand! :D

patrickhulce · 2021-05-12T16:57:09Z

lighthouse-core/lib/network-request.js

+   * @param {number} [timeout]
+   * @return {Promise<string>}
+   */
+  static async getRequestContent(session, requestId, timeout = 1000) {


while we're in the reorg mode...WDYT about fetchResponseBodyFromCache?

fetch to signal that it's an operation that takes some time over the protcol, not just a getter

FromCache to signify that it's not re-requesting anything, it's just trying to get it from memory cache (FromMemory or FromMemoryCache alternates?)

ResponseBody because it is weird that we say we're getting the request content when it's the response content, request is sort of implicit in all of these operations

maybe fetch should be reserved for things fetched over the network, though?

maybe fetch should be reserved for things fetched over the network, though?

That was the point of FromCache, an alternate suggestion? fetchFromCache has a weaker "over the network" connection to me than get's connection to "returns immediately"

I guess I always found the name descriptive enough except for the request part you mentioned, so getResponseContent seems ok to me :)

If it was on NetworkRequest I could definitely see the issue with get sounding like just a local variable, but between the async and being in a driver/* file, I'd just assume it's doing something over the protocol, but maybe that's just because I already know how it works.

how's about getResponseContentFromCache then? the current method makes it sound like the content will be available much more often than it actually is (if we're making wishes I'd also want to have this | undefined like our DOM utilities do when this thing throws).

patrickhulce

impl looks great!

lighthouse-core/test/gather/gatherers/main-document-content-test.js

patrickhulce · 2021-05-12T17:02:03Z

lighthouse-core/test/gather/gatherers/main-document-content-test.js

+jest.mock('../../../computed/network-records.js');
+
+describe('FR compat', () => {
+  /** @type {MainDocumentContent} */


a trick I like, is to just double initialize these (in declaration and beforeEach) to avoid duplicating the type annotation

e.g. let gatherer = new MainDocumentGatherer();

(don't need to change it, just offering an alternative that comes in handy when the types get more complex :)

patrickhulce · 2021-05-12T17:03:16Z

tsconfig.json

@@ -52,6 +52,7 @@
    "lighthouse-core/test/gather/gatherers/http-redirect-test.js",
    "lighthouse-core/test/gather/gatherers/js-usage-test.js",
    "lighthouse-core/test/gather/gatherers/link-elements-test.js",
+    "lighthouse-core/test/gather/gatherers/main-document-content-test.js",


nooooo, but you've been so great about all the tests so far, and this is a nice opportunity to expand! :D

patrickhulce

B-E-A-UTIFUL

lighthouse-core/gather/driver/network.js

patrickhulce · 2021-05-13T03:01:56Z

lighthouse-core/test/fraggle-rock/gather/mock-driver.js

@@ -127,11 +127,34 @@ function mockDriverModule(driverProvider) {
  };
 }

+function createMockContext() {


patrickhulce · 2021-05-13T03:05:05Z

lighthouse-core/test/gather/gatherers/main-document-content-test.js

+const devtoolsLog
+  = /** @type {LH.DevtoolsLog} */ (require('../../fixtures/traces/lcp-m78.devtools.log.json'));
+
+describe('FR compat', () => {


oh this file

Co-authored-by: Patrick Hulce <[email protected]>

adamraine · 2021-05-13T15:54:08Z

I'm going to merge because I want to use createMockContext in the next PR. I can do a follow up if there are any strong opinions on naming/location of things.

brendankenny · 2021-05-13T15:57:02Z

I can do a follow up if there are any strong opinions on naming/location of things.

network.js wfm and the rest is just bikeshedding :)

adamraine added 2 commits May 12, 2021 12:42

core(fr): convert main-document-content gatherer

39dd59c

udate config

4295bdf

adamraine requested a review from a team as a code owner May 12, 2021 16:50

adamraine requested review from patrickhulce and removed request for a team May 12, 2021 16:50

google-cla bot added the cla: yes label May 12, 2021

devtools-bot assigned patrickhulce May 12, 2021

devtools-bot added the waiting4reviewer label May 12, 2021

adamraine commented May 12, 2021

View reviewed changes

patrickhulce reviewed May 12, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels May 12, 2021

adamraine added 3 commits May 12, 2021 13:35

rn getRequestContent

621cf4e

typecheck tests

4e24733

move to driver

fd64f1c

vercel bot deployed to Preview May 12, 2021 20:04 View deployment

adamraine added waiting4reviewer and removed waiting4committer labels May 12, 2021

patrickhulce approved these changes May 13, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels May 13, 2021

Update lighthouse-core/gather/driver/network.js

5835f8b

Co-authored-by: Patrick Hulce <[email protected]>

vercel bot deployed to Preview May 13, 2021 14:30 View deployment

adamraine merged commit 8769ff8 into master May 13, 2021

adamraine deleted the fr-main-document-content branch May 13, 2021 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(fr): convert main-document-content gatherer #12470

core(fr): convert main-document-content gatherer #12470

adamraine commented May 12, 2021

adamraine May 12, 2021

patrickhulce May 12, 2021 •

edited

Loading

adamraine May 12, 2021

brendankenny May 12, 2021

brendankenny May 12, 2021 •

edited

Loading

patrickhulce May 12, 2021

brendankenny May 12, 2021

patrickhulce May 12, 2021

adamraine May 12, 2021

adamraine May 12, 2021

patrickhulce May 12, 2021

patrickhulce May 12, 2021

brendankenny May 12, 2021

patrickhulce May 12, 2021

brendankenny May 12, 2021

patrickhulce May 12, 2021

patrickhulce left a comment

patrickhulce May 12, 2021

patrickhulce May 12, 2021

patrickhulce May 12, 2021

patrickhulce left a comment

patrickhulce May 13, 2021

patrickhulce May 13, 2021

adamraine commented May 13, 2021

brendankenny commented May 13, 2021

core(fr): convert main-document-content gatherer #12470

core(fr): convert main-document-content gatherer #12470

Conversation

adamraine commented May 12, 2021

Choose a reason for hiding this comment

patrickhulce May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny May 12, 2021 • 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

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

patrickhulce 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

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine commented May 13, 2021

brendankenny commented May 13, 2021

patrickhulce May 12, 2021 •

edited

Loading

brendankenny May 12, 2021 •

edited

Loading