-
-
Notifications
You must be signed in to change notification settings - Fork 937
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
adds test cases for swapping objects/arrays on stores #2281
base: main
Are you sure you want to change the base?
Conversation
|
Yeah I admit I don't know I considered ever supporting this. Is it even possible? Because we can't change the nature of the topmost proxy. We connect it to an object or array and we need to keep that reference. |
The test seems to be intended to change the property I'm not suggesting, but theoretically I suppose you can proxy a |
Yeah I read this wrong. This should be workable. EDIT.. actually I think I was right in the first place. Like |
ok, I understood how this works, simplifying It's something on the lines of: // store.value === {}
setStore('value', reconcile([1, 2]))
function setStore(key, reconcile){
// reconcile receives `store.value` which is `{}`
reconcile(store[key])
}
function reconcile(newValue) {
// newValue is `[1, 2]`
return state => {
// state is `{}`
state = newValue
}
} I presume we can error out when |
There's this to consider #2017 (comment) setStore("value", {0: something }); Kind of an awkward api if you ask me |
Yeah I just don't know if it can be "solved". Typical
To me these are both isolated considerations. There could be a future where we only mutably change stores.. like |
I understand the identity is to be preserved, but in my opinion this edge case is about losing that identity. It would be on the same lines as patching an array that has some objects with an empty array, the identity of the objects will vanish because the array It's now empty, so it's expected to lose information. From I'm considering this edge case could happen not intentionally by the developer but when patching objects from API results (idk I am trying to imagine a situation). The issue I have with this, is that if stuff changes from object to array, then the API of these objects change (imagine trying to |
I guess the only potential bug here from my perspective is that an object can be used to set a previously undefined key. const [store, setStore] = createStore({ value: null });
// should throw?
setStore("value", { something: "hey" }) Now the patch becomes the object. In a sense an object passed into This however would conceptually be completely fine: const [store, setStore] = createStore({ value: null });
setStore({ value: { something: "hey" } }) Here you are assigning a new value to I'm not going to say this is the most intuitive API but it is what the DSL is. From that perspective there is no problem with: const [store, setStore] = createStore({ value: [] });
// update index 0
setStore("value", { 0: "hey" }) Which is completely different than: const [store, setStore] = createStore({ value: [] });
// replace value with an object
setStore({ value: { 0: "hey" } }) |
Thanks, that's very insightful. I've been thinking of this from the perspective of the user, as in |
Summary
Was reading the
isArray
condition onapplyState
.. investigating the issues here found this comment #1973 (comment)This other post may be relevant #2017 (comment) I haven't considered you could patch an array at an index with an object describing the patch.
Admittedly, needing to swap object types seems weird, don't remember having run into this myself. In any case this PR adds two failing test cases for it. I tried to give it a shot but couldn't pass the test, so Im leaving this here.