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

[Improvement] Add a shallow merge option to Onyx.merge #87

Open
thienlnam opened this issue Jul 12, 2021 · 1 comment
Open

[Improvement] Add a shallow merge option to Onyx.merge #87

thienlnam opened this issue Jul 12, 2021 · 1 comment

Comments

@thienlnam
Copy link
Contributor

Problem

The default behavior of Oynx.merge is a deep merge which will object keys and array elements. This is fine but for some scenarios, we want to overwrite the object entirely while.

For example, to merge this data

{
  A: {email: '[email protected]', description: 'this is a description'}
}

into this existing onyx key

{
  A: {email: '[email protected]', description: '', test: 'test', stuff: 'stuff'}
  B: {email: '[email protected]', description: 'this is a description'}
  otherKey: bla
  ...  
}

Instead of getting a new value of
A: {email: '[email protected]', description: 'this is a description', test: 'test', stuff: 'stuff'}
we want to get this saved in the onyx key

{
  A: {email: '[email protected]', description: 'this is a description'}
  B: {email: '[email protected]', description: 'this is a description'}
  otherKey: bla
  ...  
}

Solution

Create an option for shallow merging which will merge the values but will overwrite objects
Also could look into doing something like Onyx.removeProperty('keyName', ['A'])

Workarounds

Right now, the workaround is to first merge the key with null, and then do another merge with the new object as the Onyx.merge calls are batched and should

cc @marcaaron

@marcaaron
Copy link
Contributor

marcaaron commented Oct 26, 2021

Just adding some more context here. A specific example of this workaround looks like this:

// Given {errors: {someError: true}}
Onyx.merge(key, {errors: null});
Onyx.merge(key, {errors: {someOtherError: true}}); // -> {errors: {someOtherError: true}}

Since otherwise a typical deep merge would give us {errors: {someError: true, someOtherError: true}}

Shallow merge could work, but I think the most flexible one is to add a callback to Onyx.set(). This also would solve the issue where array values do not get concatenated in a deep merge. But the current workaround for that has us relying on local data e.g. here in Policy.js.

So instead we'd end up with

Onyx.set((previous) => ({
    ...previous,
    errors: {someOtherError: true},
}))

And

Onyx.set(key, (previousPolicy) => ({
    ...previousPolicy,
    employeeList: _.without(previousPolicy.employeeList, ...members),
}));

The only thing blocking me here is that a set() is more likely to cause a race condition than a merge() if called consecutively. So, I want to:

  • See if we are actually doing that and add logs ✅
  • Make a set() always cancel any queued merge() that happen before them in the same call
  • Implement the set() with callback

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

No branches or pull requests

2 participants