adds test cases for swapping objects/arrays on stores

[titoBouzout]

Summary

Was reading the isArray condition on applyState .. 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.

[changeset-bot]

⚠️ No Changeset found

Latest commit: ea99dd6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[ryansolid]

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.

[titoBouzout]

The test seems to be intended to change the property .value, seems doable if the proxy is on the parent. I am not sure if on the implementation it is considered as topmost proxy. I get the idea that some stuff it's tracked by returning new proxies to the data, but how all plays together is complicated to think about.

I'm not suggesting, but theoretically I suppose you can proxy a listener object, hold a reference to it, and then swap the underlying data by changing its prototype. Something like this https://playground.solidjs.com/anonymous/e2ab459b-7c0c-4583-9c72-7ca431cf100c (cant find a way to fool Array.isArray so probably has downsides)

[ryansolid]

Yeah I read this wrong. This should be workable.

EDIT.. actually I think I was right in the first place. reconcile only knows what you pass to it. So you should be able to reconcile something with a nested value property, but I'm not sure we can if the value prop is outside and you are passing in top level array or object to reconcile.

Like setStore(reconcile()) should work in this case but setStore("value", reconcile()) can't be cause it's input is already of that type and can't be changed except from the outside.

[titoBouzout]

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 state and newValue are both object, but one it's an Array and the other isn't.

[titoBouzout]

There's this to consider #2017 (comment)
Basically reconcile can patch an array at an index with an object that describes the patch

setStore("value", {0: something });

Kind of an awkward api if you ask me

[ryansolid]

Yeah I just don't know if it can be "solved".

Typical setStore an object is a patch instruction since we can't change the identity of the proxy itself top-level. If objects were not to merge we couldn't accept top level objects. In our case it happened that it started that way (based on React's setState) and continued out.

reconcile is a different beast. It is supposed to transform what is there to be exactly what you pass in. But we can't change the identity of the underlying proxy. We can change nested properties but we can change an array into an object or vice versa, otherwise we'd need to set a new proxy on all parents, which we can't get to.

To me these are both isolated considerations. There could be a future where we only mutably change stores.. like produce by default and there again you still won't be able to change the nature of top-level proxy even without merging. But reconcilestill will have this limitation. As long as you would want to be able to reconcile top-level (I believe we need to) then we don't have a way to change the identity of the top-most thing passed in.

[titoBouzout]

Typical setStore an object is a patch instruction since we can't change the identity of the proxy itself top-level.

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 [{u:1}, {u:2}, {u:3}] to [] there's no identity to preserve for the ù objects.

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 .push into an object) it will error out unexpectedly. [edit] ok maybe you shouldn't be pushing to an array in a store, but you get the idea of what I am thinking.

[ryansolid]

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 setStore should be seen as an assign operation. So you can't assign something to null.

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 value.

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" } })

[titoBouzout]

Thanks, that's very insightful. I've been thinking of this from the perspective of the user, as in <>{store.value}</>. While I wouldn't have expected the first example to throw I would welcome throwing on the cases that it can't handle. So maybe from my suggestion it can throw when trying to set an array when it's an object, but accept an object when its an array (as its a describing patch) [unless I am misunderstanding]