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

proto: update processForProto method signature, string -> LH.Result #9016

Merged
merged 8 commits into from
Jun 11, 2019

Conversation

paulirish
Copy link
Member

Also, lightrider will only request JSON output now.

@brendankenny
Copy link
Member

does this supersede #9002?

@brendankenny
Copy link
Member

does this supersede #9002?

oh, jk, only touching results.lhr

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

some nits, but LGTM otherwise

lighthouse-core/lib/proto-preprocessor.js Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@
*/
'use strict';

const processForProto = require('../../lib/proto-preprocessor').processForProto;
Copy link
Member

Choose a reason for hiding this comment

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

can you add a test that it doesn't modify the passed-in lhr?

* @param {{lrDevice?: 'desktop'|'mobile', categoryIDs?: Array<string>, logAssets: boolean, configOverride?: LH.Config.Json}} lrOpts Options coming from Lightrider
* @return {Promise<string|Array<string>|void>}
* @return {Promise<string|void>}
Copy link
Member

Choose a reason for hiding this comment

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

is void still a return possibility? (I think it can be dropped now)

Copy link
Member

Choose a reason for hiding this comment

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

is void still a return possibility?

Oh, yes it is for the if (!results) return case. Should it throw to get a runtime error when that happens? Weird to return nothing if Lighthouse was acting poorly (I think it's actually a // should never happen case to make tsc happy).

@@ -25,9 +25,9 @@ const LR_PRESETS = {
* If configOverride is provided, lrDevice and categoryIDs are ignored.
* @param {Connection} connection
* @param {string} url
* @param {LH.Flags} flags Lighthouse flags, including `output`
* @param {LH.Flags} flags Lighthouse flags
Copy link
Member

Choose a reason for hiding this comment

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

make sure you aren't accidentally still passing in output: 'html' and doing all that extra work :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment.. i dont think we need a stronger check.

Copy link
Member

Choose a reason for hiding this comment

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

Added a comment.. i dont think we need a stronger check.

yeah, I meant just double check existing code, definitely don't need a check in the code

/** @type {LH.Result} */
const reportJson = JSON.parse(result);
function processForProto(lhr) {
const reportJson = JSON.parse(JSON.stringify(lhr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we skip the cloning? Only the lr entry uses this fn, and the original lhr is not needed there. if some other client comes along with different requirements they can clone the input themselves.

assuming cloning a lhr takes a measurable amount of time.

I clocked 4ms for example.com and for 8ms nyt.com:

console.time('clone'); JSON.parse(JSON.stringify(lhr)); console.timeEnd('clone');

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it unexpected when methods mutate the passed in object, so this is my preferred pattern. It's also what we do for Util.prepareReportResult, swapLocale, our tests, etc.

We adopted this pattern in our tests, as previously we had some unexpected mutations that caused failures that were tricky to track down.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I prefer avoiding mutation too. if the few ms is worth the change, I'd suggest renaming the method to modifyForProto to properly set expectations.

@googlebot

This comment has been minimized.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

some comments on comments and need to fix the type error, but otherwise LGTM.

Love the simplification!

@@ -25,9 +25,9 @@ const LR_PRESETS = {
* If configOverride is provided, lrDevice and categoryIDs are ignored.
* @param {Connection} connection
* @param {string} url
* @param {LH.Flags} flags Lighthouse flags, including `output`
* @param {LH.Flags} flags Lighthouse flags
Copy link
Member

Choose a reason for hiding this comment

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

Added a comment.. i dont think we need a stronger check.

yeah, I meant just double check existing code, definitely don't need a check in the code

@@ -38,6 +38,7 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) {
flags.disableStorageReset = true;
flags.logLevel = flags.logLevel || 'info';
flags.channel = 'lr';
// flags.output assumed to be unset by Lightrider
Copy link
Member

Choose a reason for hiding this comment

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

yeah, probably just drop this line. At worst it's just extra work, and is just as assumed as every other property on flags could be (and we don't make a comment for any of those)

Copy link
Collaborator

Choose a reason for hiding this comment

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

should it instead throw if output is set to anything other than undefined or json? seems like it could be frustrating to figure that out on your own

but you both seem to be on the same page that we "definitely don't need a check in the code" so I feel like I'm missing something

Copy link
Member

Choose a reason for hiding this comment

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

should it instead throw if output is set to anything other than undefined or json?

yeah, that's why I was suggesting removing the comment, because from this comment it definitely sounds like it should be an assertion, not a comment :)

AIUI, the runnerResult.report is ignored completely on the LR side now, it uses the LHR directly (with its own HTML reportGenerator bundle), so at worst a defined output is just extra work.

...but there are several possible things on flags that could add extra work and we're just assuming those are correct too, so I really don't think it's worth thinking about. Really it's something for the caller to worry about, either way anyways


return preprocessor.processForProto(runnerResult.report);
// When LR is called with |internal: {keep_raw_response: true, save_lighthouse_assets: true}|,
// this code will log artifacts to raw_response.artifacts.
Copy link
Member

@brendankenny brendankenny Jun 6, 2019

Choose a reason for hiding this comment

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

this comment is inherited but is pretty confusing here since it appears to refer entirely to stuff outside of LH? e.g. keep_raw_response and/or save_lighthouse_assets somehow become lrOpts.logAssets, and lhr becomes raw_response?

If it's helpful for users of lr, though, seems ok to keep

// When LR is called with |internal: {keep_raw_response: true, save_lighthouse_assets: true}|,
// this code will log artifacts to raw_response.artifacts.
if (logAssets) {
// @ts-ignore - Regenerate the report, but tack on the artifacts.
Copy link
Member

Choose a reason for hiding this comment

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

maybe regenerate was referring to what it used to be doing? what about just // @ts-ignore - piggyback the artifacts on the LHR.

/** @type {LH.Result} */
const reportJson = JSON.parse(result);
function processForProto(lhr) {
const reportJson = JSON.parse(JSON.stringify(lhr));
Copy link
Member

Choose a reason for hiding this comment

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

the type error (failure in travis) is because this whole function is operating on an any reportJson (due to the return type of JSON.parse). Luckily there was a forEach in there or we wouldn't have noticed the whole thing wasn't being checked :)

Need to keep the /** @type {LH.Result} */ above this line.

@exterkamp exterkamp added cla: yes and removed cla: no labels Jun 7, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

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!

I guess the no more HTML in LR issue was prediscussed I missed? Because that seems like the much bigger visible change to me personally as an outsider 😆

@@ -38,6 +38,7 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) {
flags.disableStorageReset = true;
flags.logLevel = flags.logLevel || 'info';
flags.channel = 'lr';
// flags.output assumed to be unset by Lightrider
Copy link
Collaborator

Choose a reason for hiding this comment

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

should it instead throw if output is set to anything other than undefined or json? seems like it could be frustrating to figure that out on your own

but you both seem to be on the same page that we "definitely don't need a check in the code" so I feel like I'm missing something

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

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.

6 participants