API: Settings: delete keys that are set to null #5830
Conversation
This fixes issues where WSL integrations could not be disabled. Signed-off-by: Mark Yen <[email protected]>
| * Normalize WSL integrations configuration. | ||
| * @param integrations The source collection, containing all WSL integrations. | ||
| * @param mode How normalization should take place | ||
| * @returns Returns a new object with normalized WSL configuration. |
There was a problem hiding this comment.
Could you defined what normalization actually means?
For example, the v || null in the submit function maps false to null, and I don't understand why you want this.
| @@ -511,10 +511,10 @@ export default class SettingsValidator { | |||
| return false; | |||
| } | |||
|
|
|||
| let changed = false; | |||
| let changed = Object.keys(currentValue).some(k => !(k in desiredValue)); | |||
There was a problem hiding this comment.
If you're setting changed here couldn't it be a constant, and we don't need the changed ||= ... bit at line 520 ?
There was a problem hiding this comment.
This only covers where the current value is a superset of desired value; the other part covers where they both have a key, but it's different.
There was a problem hiding this comment.
When I disable an integration, it's completely removed from the settings {... WSL: { integrations: {...}}} instead of staying there but set to false.
If I wanted to manage my integrations from rdctl or the API, it's harder, because I no longer see existing but disabled integrations. I don't see why this would be a requirement.
Also a couple of comments about the code...
| diff: (entries: [string, boolean][]) => entries.filter(([, v]) => v === true), | ||
| submit: (entries: [string, boolean][]) => entries.map(([k, v]) => [k, v || null] as const), | ||
| }[mode]; | ||
| const someVal = Object.fromEntries(normalizeFn(Object.entries(integrations))); |
There was a problem hiding this comment.
Can just return the result of Object.fromEntries(...) (Same comment for the code that this PR replaces)
There was a problem hiding this comment.
Yep, totally doable.
| ); | ||
| const normalizeWslIntegrations = (integrations: Record<string, boolean>, mode: 'diff' | 'submit') => { | ||
| const normalizeFn = { | ||
| diff: (entries: [string, boolean][]) => entries.filter(([, v]) => v === true), |
There was a problem hiding this comment.
If v is boolean, can we not just say …(([, v]) => v), ?
There was a problem hiding this comment.
Yep, was just keeping what the old code did. Changed.
Also simplify the implementation slightly. Signed-off-by: Mark Yen <[email protected]>
mook-as
force-pushed
the
wsl/integration-disable
branch
from
ce1b6f0
to
4072065
Compare
| @@ -511,10 +511,10 @@ export default class SettingsValidator { | |||
| return false; | |||
| } | |||
|
|
|||
| let changed = false; | |||
| let changed = Object.keys(currentValue).some(k => !(k in desiredValue)); | |||
There was a problem hiding this comment.
This only covers where the current value is a superset of desired value; the other part covers where they both have a key, but it's different.
| * Normalize WSL integrations configuration. | ||
| * @param integrations The source collection, containing all WSL integrations. | ||
| * @param mode How normalization should take place | ||
| * @returns Returns a new object with normalized WSL configuration. |
This fixes issues where WSL integrations could not be disabled.
Fixes #5805