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

Modification object in updating hook not correct when changing array #1270

Closed
Christilut opened this issue Apr 12, 2021 · 1 comment
Closed

Comments

@Christilut
Copy link

See https://jsfiddle.net/ygaprkbe/2/

In the console you will see {} but it should be the object in the update call.

So far I've only seen this when updating items inside of an array.

Also, most likely a related bug: https://jsfiddle.net/ygaprkbe/3/

The modifications object here is { foo.bar: 444 } which seems similar to this old bug #1195

@dfahlander
Copy link
Collaborator

dfahlander commented Apr 12, 2021

Thanks! I see what's wrong here. Arrays of objects compare equal due to getValueOf() only supporting arrays of simple types. The commit that fixed #1195 did compare arrays using getValueOf(). Might need to extend getValueOf() to support objects and go nested.

TODO:

  1. Create a repro test in test-crud-hooks.js or extend the existing "issue Update with array as value adds number objects #1195 Update with array as value adds number objects".
  2. Extend getValueOf() to support "Object". Would be tempting to do JSON.stringify() but that wouldnt support binary types so better to do a nested call to getValueOf().

dfahlander added a commit that referenced this issue Jun 10, 2021
Did simplify this a bit and remove some code.
We only need to go deep diffing when both ap and bp are POJOs. All else can be treated as diff.
This means that updating an object with an array property that has identical content but is another array instance, will be treated as a diff. This is ok. I don't think we earn so much by using getValueOf() to do some half-baked comparision between arrays.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants