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(lhr): move runtime config to report, configSettings #5122

Merged
merged 5 commits into from
May 6, 2018

Conversation

patrickhulce
Copy link
Collaborator

  • removes runtimeConfig from LHR
  • adds runtimeSettings to LHR (I'd prefer just .settings but doing the rename of that back to something else would have been more challenging, so lmk if other folks feel the same I'd I'll happily change :D)
  • moves the emulation description logic to the report renderer

I know that this whole thing is ripe for some refactoring, but we've got a long list to get through and this keeps same functional parity, sooooo

image

NOTE: closure fixes pending

@paulirish paulirish mentioned this pull request May 4, 2018
82 tasks
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.

works for me. naming proposal tho.

],
"blockedUrlPatterns": [],
"extraHeaders": {}
"runtimeSettings": {
Copy link
Member

Choose a reason for hiding this comment

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

wdyt configSettings? that's what we call them elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sg done

@patrickhulce patrickhulce changed the title core(runtimeConfig): move runtime config to report core(configSettings): move runtime config to report May 6, 2018
@patrickhulce patrickhulce changed the title core(configSettings): move runtime config to report core(lhr): move runtime config to report, configSettings May 6, 2018
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.

LGTM2!

One surprise people may have is extraHeaders being saved in the LHR/report if they're using them for authentication, but I guess the URL of their dev server or whatever is already in there and if they're comfortable sharing a report with that in it...

We could make the headers (if any) visible in the "Runtime settings" section of the report as a warning that they're in the file, though that's collapsed by default so it might not add much.

],
"blockedUrlPatterns": [],
"extraHeaders": {}
"configSettings": {
Copy link
Member

Choose a reason for hiding this comment

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

this is going to be great to have

@brendankenny
Copy link
Member

I know that this whole thing is ripe for some refactoring

did you mean in how we generate things in getEmulationDesc or more? Make sure to make a note of it :)

@paulirish paulirish merged commit 46d11af into master May 6, 2018
@paulirish paulirish deleted the runtime_config branch May 6, 2018 21:02
kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants