optimize merge defaults strategy #516
Conversation
alexzurbonsen
force-pushed
the
fix-mergeObjects-function
branch
2 times, most recently
from
4dff3f6 to
b1d0318
Compare
src/lib/compute.ts
Outdated
| @@ -21,7 +21,7 @@ const mergeDefaults = ( | |||
| ) => { | |||
| if (inputs) { | |||
| const response = defaults | |||
| ? inputs.map(input => mergeObjects(input, defaults)) | |||
| ? inputs.map(input => mergeObjects({...defaults}, input)) | |||
There was a problem hiding this comment.
Hi @alexzurbonsen , thanks for your contribution.
Just wandering if there is specific reason for using object destructuring on defaults?
There was a problem hiding this comment.
Hi @MariamKhalatova, that is because the function uses lodash's mergeWith function. And that writes input into defaults.
Not sure how much more detail the explanation should contain, but just in case, here is the rest: Because objects are passed by reference in JS we need to deep copy the top level of defaults here. And that is what the spread operator does for us.
There was a problem hiding this comment.
I added a test, that might help understand the issue. If you replace {...defaults} with defaults and run the test, you can see how the object reference kicks in.
I had to export mergeDefaults to make this testable. But I guess its worth it.
Currently the mergeObject function has a couple of problems. E.g. it overrides an input if its value is false and the default is true. It does not deep merge objects. Added a test suite that the current function would not pass. And fixed it. Signed-off-by: alexzurbonsen <[email protected]>
alexzurbonsen
force-pushed
the
fix-mergeObjects-function
branch
from
b1d0318 to
f000d2d
Compare
|
Thank you for your comments. I adressed them. |
|
Hi @alexzurbonsen, we have gone through this PR on our weekly triage. Everything seems good and we are keen to merge your PR if it can be rewritten without |
|
Hi @narekhovhannisyan, to be honest I am not a fan of this approach in this case. Writing this kind of code is fairly complex or at least complex enough that you easily make mistakes or overlook edge cases. In this kind of situation I would prefer to use a library. The function we are fixing in this PR is a good example, why it isn't a good idea to write this stuff yourself. Thus, I won't invest time to rewrite a lodash function that works better than any code I will be writing myself. |
|
But of course, you can take over the PR if you like. |
|
@alexzurbonsen we decided to not merge arrays and nested objects to keep everything straightforward. Will push changes for tests and the implementation accordingly. |
src/util/helpers.ts
Outdated
| export const mergeObjects = (defaults: any, input: any) => { | ||
| const merged: Record<string, any> = {...input}; | ||
|
|
||
| for (const key in defaults) { | ||
| if (Array.isArray(defaults[key])) { | ||
| merged[key] = input[key] !== undefined ? input[key] : defaults[key]; | ||
| } | ||
| return; | ||
| }; | ||
|
|
||
| return mergeWith(destination, source, handleArrays); | ||
| if (!(key in input)) { | ||
| merged[key] = defaults[key]; | ||
| } | ||
| } | ||
|
|
||
| return merged; |
There was a problem hiding this comment.
Why not just use {...defaults, ...inputs} now?
There was a problem hiding this comment.
it won't do the trick since null values won't be overridden
There was a problem hiding this comment.
and that behavior should be covered by tests too, the current implementation doesn't satisfy that either
I guess it might be helpful if this behaviour is documented somewhere? |
narekhovhannisyan
changed the title
MariamKhalatova
merged commit f482eb0
into
Green-Software-Foundation:main
Types of changes
A description of the changes proposed in the Pull Request
I wrote a test suite that explains my understanding of what the function should do. The current
mergeObjectfunction fails this suite. E.g. it overrides an input if its value is false and the default is true. It does not deep merge objects.Changed the implementation to pass the tests.