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

Make settings type flat #175

Closed
justinkambic opened this issue Apr 23, 2020 · 0 comments · Fixed by elastic/kibana#64856
Closed

Make settings type flat #175

justinkambic opened this issue Apr 23, 2020 · 0 comments · Fixed by elastic/kibana#64856
Assignees
Labels
technical debt test-plan test-plan-ok Indicates an issue has been tested for release v7.8.0

Comments

@justinkambic
Copy link

We have a type defined for our application's settings, but a nested data structure (like the one we use today) inherently adds complexity when modifying slices of state. We can't simply Object.assign our state changes overtop of the existing settings values, because if only one field is modified, the unmodified fields in a nested object are overwritten.

This leads to us doing weird things like using _.merge, which carries its own quirkiness because merge isn't pure and modifies its parameter vars directly. When comparing to things like the returned values from React.useState, which should be immutable, it creates weird side effects that are painstaking to debug.

We're solving it now by explicitly referencing the fields on the settings type to updated them, like below:

      setFormFields({
        heartbeatIndices: changedField.heartbeatIndices ?? formFields.heartbeatIndices,
        certThresholds: Object.assign(
          {},
          formFields.certThresholds,
          changedField?.certThresholds ?? null
        ),
      });

This is a pure method of handling state change and it works fine, but obviously it isn't scalable. The more fields we add to settings down the line, the more complex any snippet of code doing this will become.

The obvious solution is to flatten our settings fields. There will likely not be that many fields, and it would be best to nail this down before we release 7.8, because we haven't released any code yet that uses nested fields. Then we will be ok to simply Object.assign({...oldState, ...newState}) anytime we modify settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical debt test-plan test-plan-ok Indicates an issue has been tested for release v7.8.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants