EthanStandel / deepsignal

113 stars 5 forks source link

Does not seem to work when props aren't known ahead of time #17

Closed Sleepful closed 9 months ago

Sleepful commented 1 year ago

This works fine with simple @preact/signals:

export const cartStore = {
  data: signal({} as { [key in symbol]: number }),
  addLicense(id: symbol) {
    this.data.value = { ...this.data.value, [id]: 1 };
  },
  changeQuantity(id: symbol, quant: number) {
    if (this.data.value[id]) {
      this.data.value = { ...this.data.value, [id]: quant };
    }
  },
  removeLicense(id: symbol) {
    this.data.value = { ...this.data.value, [id]: 0 };
  },
};

Is there a way to have similar or better functionality with deepsignal?

EthanStandel commented 9 months ago

Sorry to be so late to this (and I will try my best to be faster in the future). I do see why this is happening. When the .value of a DeepSignal is set, I specifically go find all the underlying Signals and write to them directly.

This is kind of curious, because the simple solution here would be for me to offer some kind of special wrapper around objects so that they won't be converted into DeepSignals (probably just by adding a secret non-enumerable property to check).

Alternatively, I could ensure that when you set a deepSignal, if there are any new properties being added, I could run the deepSignal construction process on them and then append them to the structure to be consumed by new listeners... but does this makes sense to do? If you're working with a data structure, then you almost definitely have to iterate over it, which means that you're getting VDOM rerenders when the structure itself is updated... so there's not a huge benefit in having the underlying signals in a dynamic structure like this. But I don't think it hurts anything, and this would be the most natural approach. So I think I'll give a shot at this solution this week.

Will comment back on results.

EthanStandel commented 9 months ago

So unfortunately, I don't believe I can't ensure that a deepSignal can have properties added to it. It seemed like a simple change from the setter, but the problem is: DeepSignals don't manage their own tracking/propagation. The tracking/propagation for DeepSignals relies entirely on the underlying Signals.

So if you wanted to create cartStore as a deepSignal, it would look like this

const cardStore = deepSignal({
  data: {}
})

Within that structure, there are no atomic properties. So if you try to iterate over the structure, with cardStore.data.value in a component... there would be nothing to track, because there's no atomic properties. So you'd never actually call a .value on an actual Signal.

So to solve this problem, I believe I'm going to have to add an atomic function which makes it so that particular objects are treated as atomic properties rather than recursed into. So this will look like this in your case, where all properties under data will not be created as a deepSignal.

const cardStore = deepSignal({
  data: atomic({})
})

If you wanted your internal items to be DeepSignals or Signals as well, I don't believe there would be anything stopping you from doing something like this.

cardStore.data.value = { ...cardStore.data.peek(), foo: deepSignal({ id: "123", quantity: 1 }) };

Which is to say, storing nested DeepSignals inside an atomic structure. The only thing you'd lose out on is natural serialization because the conversion back from a DeepSignal to the value will end at whatever is declared to be an atomic property.

EthanStandel commented 9 months ago

Alright, so scratch that previous comment. I was able to make the modifiable structure works. Now every DeepSignal will have a __structure property which is a signal of exactly the last payload that was applied to it. That way the stucture itself can be listened to at any level and not just the atomic data.

PR: https://github.com/EthanStandel/deepsignal/pull/19

I'm going to release this in the alpha tag and play around with it some more. The only thing I don't like about this solution is that in the case below in the current version, there would be no updates or rerenders because I would only ever set the individual signals and the built in primitive equality check on signals would prevent any unecessary renders. But with this change, the signal representing the structure will always re-fire. I'd like to prevent this, but it's also probably not a huge dealbreaker for anyone.

const store = deepSignal({ hello: { world: 1 } });
store.hello.value = { world: 1 };
EthanStandel commented 9 months ago

Version 4.0.0-alpha.2 should be everything you're looking for. I was able to clean up the performance stuff by making __structure only ever update if the structure changes. Otherwise it will remain stale. It should support additions + deletions to Record types within DeepSignals. I'm going to actually create a small example app to do some visual testing for my own comfort (though the packages all do have 100% test coverage), and then otherwise I'll update the docs with the new features and deploy this version as 4.0.0 later this week.

EthanStandel commented 9 months ago

All my testing is complete and the changes appear stable. This update is now in version 4.0.0

Thank you for your recommendation!