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.

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

core(gather): add new MainDocumentContent public artifact #9781

Merged
merged 4 commits into from
Oct 16, 2019

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Oct 3, 2019

fixes #9756
part of #9774

Happy to bikeshed on name. Some variation of MainResource or MainHtml would match our naming for finding/dealing with the request itself, but doesn't sound very good to me (a main request, yes, but what's the "main html" if e.g. you're an app that loads 90% of your content one or two steps down the critical request chain?).

InitialHtml isn't the best, but it does nicely describe that it's the initial html, regardless of what JS does to it as the page is loaded.

@@ -39,7 +39,7 @@ const smokeTests = [{
}, {
id: 'dbw',
expectations: require('./dobetterweb/dbw-expectations.js'),
config: require('./dbw-config.js'),
config: require('../../../lighthouse-core/config/default-config.js'),
Copy link
Member Author

Choose a reason for hiding this comment

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

based on #9765 (comment) it seemed time to just embrace it for dbw_tester and have it load them all.

Only adds like 2 extra seconds for the offline pass (which was skipped before but now quickly does nothing)

lighthouse-cli/test/smokehouse/dbw-config.js Outdated Show resolved Hide resolved
@brendankenny brendankenny mentioned this pull request Oct 3, 2019
54 tasks
@patrickhulce
Copy link
Collaborator

@khempenius was just writing up a document on what to name this thing for budgets. Maybe she'd be willing to shed some light here too :)

@benschwarz
Copy link
Contributor

benschwarz commented Oct 3, 2019

Happy to bikeshed on name. Some variation of MainResource or MainHtml would match our naming for finding/dealing with the request itself

Maybe rather than main (because that's "inside lighthouse" nomenclature) it could be PageHTML. That (to me at least) describes that it's sourced from the main resource and the HTML of the page that someone views. 🤔

@khempenius
Copy link
Collaborator

When I was bike shedding about this recently I found myself wavering between "initialDocument" and "mainResource."

I have mixed feelings about using html in the name. On one hand it's a very widely recognized concept, OTOH I'm not sure if the fact that it can include inlined CSS and JS will be confusing to people. I might also have a bit of bias here because LightWallet already uses "document" rather than HTML in its terminology.

@patrickhulce
Copy link
Collaborator

I'd be hesitant to go with anything that could be confused for the HTML of the rendered page. If we're using HTML as the suffix, then Initial seems like a requirement for the name IMO.

I'm partial to some of the other suggestions here that would match our usage elsewhere. MainResource matches both our internal method names that find it and the planned budgets name, so that's my favorite.

@connorjclark
Copy link
Collaborator

did you forget to git add the gatherer?

@brendankenny
Copy link
Member Author

did you forget to git add the gatherer?

haha yes, whoops

@brendankenny
Copy link
Member Author

MainResource matches both our internal method names that find it and the planned budgets name, so that's my favorite.

That might be circular, though. Is it the planned budgets name because that's what the internal methods call it? :)

How did that name come about? I have no memory of it.

My main issue with MainResource is that as a public name it's not very self describing (it could be mistaken for more of a heuristic thing like FMP), whereas everyone knows what the html of a web page is, and--for more advanced users--"initial" clarifies that it's what the html starts out as, not what it becomes after modifications.

There's not a rush on this, though. Anyone else feel free to weigh in and we can also speak our piece at the next meeting.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Oct 7, 2019

That might be circular, though. Is it the planned budgets name because that's what the internal methods call it? :)
How did that name come about? I have no memory of it.

That's a question for Katie :) I did not assume it come from that method name given the debate, but I suppose it's possible. Here's the document with some discussion in resolved comments.

EDIT: I missed a comment there where she's actually leaning away from main-resource to initial-document so my preference would be aligning on that :)

@patrickhulce
Copy link
Collaborator

also I can't pretend I didn't see 😛

snapshots are bad and they should feel bad

😡

@brendankenny
Copy link
Member Author

also I can't pretend I didn't see 😛

snapshots are bad and they should feel bad

😡

lol, maybe I should have specified that particular snapshot.

@patrickhulce
Copy link
Collaborator

maybe I should have specified that particular snapshot.

😆 alright there we can definitely agree haha

@khempenius
Copy link
Collaborator

Yep, I had initially proposed main-resource for the budget stuff. But then I realized that it's a bit of a Lighthouse colloquialism, so I switched to proposing initial-document. I don't think either is that bad of a name, but initial-document is a little bit easier to look up on MDN, etc.

@paulirish
Copy link
Member

naming: personally i'd go with something like MainDocumentContent ... -ish.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

code lgtm.
just naming stuff

lighthouse-core/gather/gatherers/initial-html.js Outdated Show resolved Hide resolved
types/artifacts.d.ts Outdated Show resolved Hide resolved
types/artifacts.d.ts Outdated Show resolved Hide resolved
@connorjclark connorjclark changed the title core(gather): add new InitialHtml public artifact core(gather): add new MainDocumentContent public artifact Oct 15, 2019
@paulirish
Copy link
Member

a conflict to resolve, otherwise this is gtg.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a html-only gatherer
7 participants