core(config): clean flags for settings #4960
Conversation
| @@ -24,11 +24,11 @@ declare global { | |||
| settings?: SettingsJson; | |||
| passes?: PassJson[]; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
someone's got a diff trailing space editor setting it seems :)
There was a problem hiding this comment.
ah, damn, eslint isn't checking .ts files, obvs. I'll find the setting :P
There was a problem hiding this comment.
an alternative to this approach would be to JSON.parse(JSON.strigify(settings)) each of them before here:
lighthouse/lighthouse-core/runner.js
Lines 186 to 187 in 0ab7775
wdyt about that instead? seems like an option, though I don't feel so strongly about it.
lighthouse-core/config/config.js
Outdated
| * @param {LH.Flags=} flags | ||
| * @return {Partial<LH.Config.Settings>} | ||
| */ | ||
| function cleanFlagsForSettings(flags = {}) { |
There was a problem hiding this comment.
semantically this is removedUndefinedProperties right?
could we call it that instead?
There was a problem hiding this comment.
not sure I'm following you, this is looking at what properties are not on constants.defaultSettings and dropping them from flags regardless of their value.
the JSON.parse(JSON.stringify()) approach would address the specific undefined yargs case but not an underlying problem that having miscellaneous junk present that isn't part of settings throws off our isDeepEqual later
There was a problem hiding this comment.
oh got it.
what properties are not on constants.defaultSettings
ahhh.. mmhmmm.
k lgtm
| @@ -26,10 +26,24 @@ const throttling = { | |||
| }, | |||
| }; | |||
|
|
|||
| /** @type {LH.Config.Settings} */ | |||
| @@ -24,11 +24,11 @@ declare global { | |||
| settings?: SettingsJson; | |||
| passes?: PassJson[]; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
ah, damn, eslint isn't checking .ts files, obvs. I'll find the setting :P
lighthouse-core/config/config.js
Outdated
| @@ -246,8 +246,24 @@ function expandArtifacts(artifacts) { | |||
| return artifacts; | |||
| } | |||
|
|
|||
| /** | |||
| * @param {LH.Flags=} flags | |||
There was a problem hiding this comment.
maybe add a function description that this is dropping any flag not found on settings?
alternative to #4958
this brings back -A, it didn't come out quite as clean as I had hoped, but it is typesafe which is kinda nice :)