-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
remove report v1 and dependencies #2596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noizzzzze
lighthouse-cli/printer.ts
Outdated
@@ -8,8 +8,8 @@ | |||
/** | |||
* An enumeration of acceptable output modes: | |||
* 'json': JSON formatted results | |||
* 'html': An HTML report | |||
* 'domhtml': An HTML report rendered client-side with DOM elements | |||
* 'html': An HTML report rendered client-side with DOM elements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just "An HTML report". "rendered client-side with DOM elements" is an unimportant implementation detail of v1 vs v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
}, | ||
details: v2TableDetails | ||
details: tableDetails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are some audits keeping extendedInfo
and others are nuking it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are some audits keeping
extendedInfo
and others are nuking it?
removing makeV1TableHeadings
was the immediate motivation, and then some that were using v2TableDetails
for details but something besides a v1 table in the extendedInfo, but ended up with identical info in each...and then I probably got a little too trigger happy. I'll revert them so we can deal with slimming down LHR in a separate PR.
assert.equal(auditResult.extendedInfo.value[0].url, URL); | ||
assert.equal(auditResult.extendedInfo.value[0].label, 'line: 123'); | ||
assert.equal(auditResult.details.items.length, 2); | ||
assert.equal(auditResult.details.items[0][1].text, URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh. we should probably rename URL
while you're here. Node 8 has https://nodejs.org/api/url.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you still have to require
it though and it's meant to be identical so why need to rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true. I suppose we'll know if someone introduces that require and things break. Just thinking ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably should generally use lowercase url
for things like that
@@ -36,7 +36,7 @@ describe('External anchors use rel="noopener"', () => { | |||
URL: {finalUrl: URL}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not clear to me why we're nuking extendedInfo in a lot of places/how the decision was made to nuke or keep. Those will be breaking changes if anyone was looking for information in there but maybe I'm missing the methodology?
@@ -247,8 +246,7 @@ class ConsistentlyInteractiveMetric extends Audit { | |||
displayValue: Util.formatMilliseconds(timeInMs), | |||
optimalValue: this.meta.optimalValue, | |||
extendedInfo: { | |||
value: extendedInfo, | |||
formatter: Formatter.SUPPORTED_FORMATS.NULL, | |||
value: extendedInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you gotta nuke the extra commas? :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you gotta nuke the extra commas? :(
haha, I realized what I was doing halfway through and thought that might be a mistake. I'll restore them :)
lighthouse-cli/printer.ts
Outdated
* 'html': An HTML report | ||
* 'domhtml': An HTML report rendered client-side with DOM elements | ||
* 'html': An HTML report rendered client-side with DOM elements | ||
* 'domhtml': Alias for 'html' report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also tweak the filename for domhtml files to be just .html
? That's in bin.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PTAL |
Should the smoketests preserve |
Yeah, I guess so, though there's no |
done |
drive-by commenters happy? |
i'm happy. |
@brendankenny you handle file move? |
got it now, thanks |
i'm good too |
also removes formatters, handlebars, handlebar build files, etc
if you're developing Lighthouse, you'll also need to manually delete you'll probably also want to |
part of #2276
also removes formatters, handlebars, handlebar build files, etc
still to do:
lighthouse-core/report/
. Was too confusing to also do that in this PR :)