solid icon indicating copy to clipboard operation
solid copied to clipboard

setting getters on already defined keys in a store loses reactivty

Open bigmistqke opened this issue 2 years ago • 2 comments

Describe the bug

Setting a getter-function on a key that has already been initialized in a store with setStore will not create a reactive getter, unlike when you set the getter-function while initializing a key.

This can be solved (as @thetarnav suggested) by doing a shallow merge.

setStore({
  alreadyDefinedKey: {
    get get() {
      return store.value;
    },
  },
});

but this feels like it should be supported without having to shallow merge objects (i personally try to avoid shallow merging as much as possible, because of the fact you can accidentally create multiple paths leading to the same reactive value and that can have unexpected results)

Your Example Website or App

https://playground.solidjs.com/anonymous/972b3aec-d7df-4ec0-9a68-c8b8b60785e3

Steps to Reproduce the Bug or Issue

const [store, setStore] = createStore({ value: 0, alreadyDefinedKey: {}});

setStore('alreadyDefinedKey', {
 get get() {
   return store.value
 }
})

setStore('newKey', {
 get get() {
   return store.value
 }
})

setStore('value', 1)

Expected behavior

I expect in the above example store.alreadyDefinedKey.get to be 1, just as store.newKey.get, but instead it is the old value 0.

Screenshots or Videos

No response

Platform

  • OS: iOS
  • Browser: firefox
  • Version: 12.1.1

Additional context

No response

bigmistqke avatar Dec 27 '22 18:12 bigmistqke

Yeah that's a reasonable request. Getters so far only work on initialization and not through the setter. Well at least that was my intention. Atleast not with a direct object set as that does a shallow merge and it doesn't do the extra work of grabbing property descriptors to decide how to merge it. We actually bind this during initialization and this won't do that with the setter. So I hadn't considered that the new key version actually worked. this was necessary for when automating memos but that had other issues... and admittedly should probably be removed for simplification at which point overhead for this could be less.

Only thing weird about setting getters is should that trigger something already listening to that property? I feel this whole getter feature needs some more consideration.

ryansolid avatar Dec 28 '22 06:12 ryansolid

We actually bind this during initialization and this won't do that with the setter. a yes true. you get access to the parent-object right? Curious if this also works when setting the new key (it does).

Only thing weird about setting getters is should that trigger something already listening to that property? I feel this whole getter feature needs some more consideration. Fair point. Imo two reasonable options: a. re-trigger when references change, b. re-trigger when references change in case of objects/arrays/... and do equality-check with primitives. I assume b. is most common usecase, but more codeheavy.

bigmistqke avatar Dec 31 '22 17:12 bigmistqke