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

merging reconcile without key duplicates the value #57

Closed
izreit opened this issue Jun 4, 2022 · 2 comments
Closed

merging reconcile without key duplicates the value #57

izreit opened this issue Jun 4, 2022 · 2 comments

Comments

@izreit
Copy link

izreit commented Jun 4, 2022

Describe the bug

After solidjs/solid#1032 I'm still playing with reconcile... let me report another issue I found.

An item unshifted is duplicated by reconcile:

const [state, setState] = createStore<any>({
  root: {
    id: "dummy",
    name: "",
    entries: []
  }
});
const root = {
  id: "id0",
  name: "ITEM 0",
  entries: [{ id: "id1",  name: "ITEM 1" }]
};
setState(reconcile({ root: root }, { key: null, merge: true }));
root.entries.unshift({ id: "id2", name: "ITEM 2 -- DUPLICATED!!!" });
setState(reconcile({ root: root }, { key: null, merge: true }));

console.log(state.root.entries[0].name); // ==> "ITEM 2 -- DUPLICATED!!!"
console.log(state.root.entries[1].name); // ==> "ITEM 2 -- DUPLICATED!!!"

(Note that the root property at the top-level is to workaround solidjs/solid#1032.)

Although I don't know the exact definition of merge: true, I think this is not an intended/expected behavior. As far as I confirmed, this duplication cannot be reproduced with one of the following modification:

  • Use push but not unshift
  • reconcile with merge: false
  • reconcile with key: "id"
  • Use a fresh new root object instead of reusing the value altered by root.entries.unshift

I guess there are some (undocumented) limitation around reusing values when merge: true... or essentially it's inappropriate for arrays modified with splice or unshift.

Your Example Website or App

https://playground.solidjs.com/?hash=575260739&version=1.4.1

Steps to Reproduce the Bug or Issue

  1. Go to the above Playground
  2. See the Result. The object including "name": "ITEM 2 -- DUPLICATED!!!" appear twice.

Expected behavior

  • ITEM 2 is not duplicated

Screenshots or Videos

No response

Platform

  • OS: Windows 11
  • Browser: Edge
  • Version: 101.0.1210.53

Additional context

No response

@ryansolid
Copy link
Member

Yeah. Internals are mutable so mutating the object outside produces unexpected results. That being said I think it's more that merge morphs everything in place and with it appearing in the object being reconciled it gets morphed to the new value.

[B, A] -> [A] = [B, B]

Because A becomes B, and then the second one is added. This is an interesting point though about merge. You should never re-use something already in the store or you risk a mutated copy. Honestly I'm not sure how merge could work any other way. If this is keyed or referential then it would insert B, and keep A.

Not sure how to formalize this limitation from a documentation standpoint. merge mutates the object in place which means any other reference mutated as well. I'm going to move this one to docs I think.

@izreit
Copy link
Author

izreit commented Jun 8, 2022

You should never re-use something already in the store or you risk a mutated copy.

Thanks now I got it. I think it is good to clarify this in docs.

For your reference I'd like to share my context why I tried to mutate the state outside store. I have a library to manipulate a tree structure data. The library is written in pure ECMAScript and has no dependencies (i.e. it doesn't know Solid). Each time the data is modified the library notifies to my app. I want to visualize this data with Solid, so I tried to reconcile it to assign to a store. But the library modified the data destructively and I got a broken store (with unexpected copy) especially while I tested merge: true.

To ensure that the values are never mutated outside store, now I have introduced a kind of viewmodel for the data to assign a store. It seems works well.

@ryansolid ryansolid transferred this issue from solidjs/solid Jun 17, 2022
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

3 participants