[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jun 11, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c508f1f
lightrider-entry can assume json now
paulirish May 21, 2019
455a20b
processForProto took/returned a string, now takes/returns LH.Result.
paulirish May 21, 2019
204c0cf
add test that input is not modified
paulirish May 25, 2019
3bf14de
Merge branch 'master' into jsoninlr
paulirish May 25, 2019
2ed7231
Update lighthouse-core/lib/proto-preprocessor.js
paulirish May 21, 2019
3a49ae2
Merge branch 'master' into jsoninlr
paulirish Jun 5, 2019
83debc0
feedback and rebase updates
paulirish Jun 5, 2019
1fe5e33
final feedback
paulirish Jun 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
lightrider-entry can assume json now
  • Loading branch information
@paulirish
paulirish committed May 21, 2019
commit c508f1f33fe9f869d5e285ccad9bc5c896ef6686
12 changes: 4 additions & 8 deletions clients/lightrider/lightrider-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

* @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).

*/
async function runLighthouseInLR(connection, url, flags, lrOpts) {
const {lrDevice, categoryIDs, logAssets, configOverride} = lrOpts;
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -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;
Expand Down