-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Changes from 2 commits
c508f1f
455a20b
204c0cf
3bf14de
2ed7231
3a49ae2
83debc0
1fe5e33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
* @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>} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, yes it is for the |
||
*/ | ||
async function runLighthouseInLR(connection, url, flags, lrOpts) { | ||
const {lrDevice, categoryIDs, logAssets, configOverride} = lrOpts; | ||
brendankenny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
@@ -59,12 +59,8 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) { | |
await assetSaver.logAssets(results.artifacts, results.lhr.audits); | ||
} | ||
|
||
// pre process the LHR for proto | ||
if (flags.output === 'json' && typeof results.report === 'string') { | ||
return preprocessor.processForProto(results.report); | ||
} | ||
|
||
return results.report; | ||
const processedLHR = preprocessor.processForProto(results.lhr); | ||
return JSON.stringify(processedLHR); | ||
} catch (err) { | ||
// If an error ruined the entire lighthouse run, attempt to return a meaningful error. | ||
let runtimeError; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,12 @@ const fs = require('fs'); | |
*/ | ||
|
||
/** | ||
* @param {string} result | ||
* Transform an LHR into a proto-friendly LHR | ||
paulirish marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param {LH.Result} lhr | ||
* @return {LH.Result} | ||
*/ | ||
function processForProto(result) { | ||
/** @type {LH.Result} */ | ||
const reportJson = JSON.parse(result); | ||
function processForProto(lhr) { | ||
const reportJson = JSON.parse(JSON.stringify(lhr)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Need to keep the |
||
|
||
// Clean up the configSettings | ||
// Note: This is not strictly required for conversion if protobuf parsing is set to | ||
|
@@ -95,7 +96,7 @@ function processForProto(result) { | |
|
||
removeStrings(reportJson); | ||
|
||
return JSON.stringify(reportJson); | ||
return reportJson; | ||
} | ||
|
||
// @ts-ignore claims always false, but this checks if cli or module | ||
|
@@ -113,9 +114,9 @@ if (require.main === module) { | |
|
||
if (input && output) { | ||
// process the file | ||
const report = processForProto(fs.readFileSync(input, 'utf-8')); | ||
const report = processForProto(JSON.parse(fs.readFileSync(input, 'utf-8'))); | ||
// write to output from argv | ||
fs.writeFileSync(output, report, 'utf-8'); | ||
fs.writeFileSync(output, JSON.stringify(report), 'utf-8'); | ||
} | ||
} else { | ||
module.exports = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
*/ | ||
'use strict'; | ||
|
||
const processForProto = require('../../lib/proto-preprocessor').processForProto; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
const {processForProto} = require('../../lib/proto-preprocessor'); | ||
|
||
/* eslint-env jest */ | ||
describe('processing for proto', () => { | ||
|
@@ -44,9 +44,9 @@ describe('processing for proto', () => { | |
'onlyCategories': null, | ||
}, | ||
}; | ||
const output = processForProto(JSON.stringify(input)); | ||
const output = processForProto(input); | ||
|
||
expect(JSON.parse(output)).toMatchObject(expectation); | ||
expect(output).toMatchObject(expectation); | ||
}); | ||
|
||
it('cleans up default runtimeErrors', () => { | ||
|
@@ -56,9 +56,9 @@ describe('processing for proto', () => { | |
}, | ||
}; | ||
|
||
const output = processForProto(JSON.stringify(input)); | ||
const output = processForProto(input); | ||
|
||
expect(JSON.parse(output)).not.toHaveProperty('runtimeError'); | ||
expect(output).not.toHaveProperty('runtimeError'); | ||
}); | ||
|
||
it('non-default runtimeErrors are untouched', () => { | ||
|
@@ -68,9 +68,9 @@ describe('processing for proto', () => { | |
}, | ||
}; | ||
|
||
const output = processForProto(JSON.stringify(input)); | ||
const output = processForProto(input); | ||
|
||
expect(JSON.parse(output)).toMatchObject(input); | ||
expect(output).toMatchObject(input); | ||
}); | ||
|
||
it('cleans up audits', () => { | ||
|
@@ -91,9 +91,9 @@ describe('processing for proto', () => { | |
}, | ||
}, | ||
}; | ||
const output = processForProto(JSON.stringify(input)); | ||
const output = processForProto(input); | ||
|
||
expect(JSON.parse(output)).toMatchObject(expectation); | ||
expect(output).toMatchObject(expectation); | ||
}); | ||
|
||
|
||
|
@@ -108,9 +108,9 @@ describe('processing for proto', () => { | |
const expectation = { | ||
'i18n': {}, | ||
}; | ||
const output = processForProto(JSON.stringify(input)); | ||
const output = processForProto(input); | ||
|
||
expect(JSON.parse(output)).toMatchObject(expectation); | ||
expect(output).toMatchObject(expectation); | ||
}); | ||
|
||
it('removes empty strings', () => { | ||
|
@@ -151,8 +151,8 @@ describe('processing for proto', () => { | |
], | ||
}, | ||
}; | ||
const output = processForProto(JSON.stringify(input)); | ||
const output = processForProto(input); | ||
|
||
expect(JSON.parse(output)).toMatchObject(expectation); | ||
expect(output).toMatchObject(expectation); | ||
}); | ||
}); |
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.
make sure you aren't accidentally still passing in
output: 'html'
and doing all that extra work :)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.
Added a comment.. i dont think we need a stronger check.
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.
yeah, I meant just double check existing code, definitely don't need a check in the code