feat(state): adds test and more

[rax-it]

No description provided.

[rax-it]

@divmain I added the microtask to notify the StateManagerSignal only once for all the notifications trickled by Atoms and Computed etc.
This might be a no-go for various reasons if we want the updates to be sync, let me know your thoughts on this.

[divmain]

The idea makes sense but I think it would have an undesired side-effect if multiple state atoms were updated simultaneously in a single updater. For each of the atoms that's updated by the updater, scheduledNotify would fire. And therefore, for each of the atoms, a microtask would be enqueued to notify downstream consumers. Additionally, we'd get redundant invocations of computeValue.

I made some suggestions for lazy evaluation of ComputedSignal; could you try implementing something similar for StateManagerSignal?

[rax-it]

@divmain The current implementation uses a flag to add a task to microtask queue only once. I like your approach of lazy evaluation of computation but it essentially still notifies state manager multiple time for example ( 2 times ) here

I was trying to avoid this essentially because ultimately this could trigger multiple DOM updates, let me know if my understanding is not correct ?

The other approach I was thinking of is if we can somehow know how many notifications an Update action would trigger but that got overcomplicated really fast.

[divmain]

I'm a bit surprised that the spy function fires twice in the linked test. Can you help me understand why that is happening?

That said, I was originally thinking that it wouldn't matter how many times a consumer is notified. This is because, when LWC consumes the signal, it has its own run-once mechanism to trigger an async re-render. So if the subscriber gets called multiple times, that's ultimately okay and would be taken care of at the web framework level.

On the other hand, while enqueuing a microtask shouldn't be strictly necessary, but it doesn't seem like bad either.

So depending on the answer to why the subscriber is called twice - I do want to know why that's happening, in case there's an internal logic bug we've overlooked - perhaps we can combine isNotifyScheduled with the lazy computation. Those two behavioral characteristics do not conflict, I don't think.

[rax-it]

@divmain

I'm a bit surprised that the spy function fires twice in the linked test. Can you help me understand why that is happening?

The state manager subscriptions should be notified for all the state changes, since internally State Manager is subscribing to notifications for individual atom/computed changes. When I perform an update increment, 2 things are changing, count and doubleCount so those 2 notifications represent those.

So if the subscriber gets called multiple times, that's ultimately okay and would be taken care of at the web framework level.

It sounds somewhat similar to the queueMicrotask approach if I am not mistaken. Trying to understand why do you think it's better to delegate this task to rendering framework instead of handling it here ?

perhaps we can combine isNotifyScheduled with the lazy computation

Agreed on this!

[divmain]

Thanks for the explanation, that is obvious in retrospect and makes complete sense.

My opinion on this comes from two things:

1. A desire for simplicity: since there is already a de-dupe mechanism in LWC, we don't need to implement it here.
2. Deduping, at least as originally implemented, involves adding some asynchronicity (with queueMicrotask - not truly async, but close enough with the complex call-stacks that might be involved) to what is otherwise synchronous.
   1. That might be entirely fine but it also might not be desirable; I'm not really sure yet.
   2. We can always make a synchronous thing asynchronous later, whereas we can't necessarily accomplish the inverse and make switch from async back to sync.

I would not be surprised if we had another PR in a short while that re-adds the queueMicrotask stuff. But I'd like to try out the simpler version first with all other pieces in the larger platform to see if we can make a more informed choice.

You know what, I'm thinking about this more and reconsidering. I don't think it adds that much complexity and the risks in my mind were probably overblown. Let's merge this PR as-is, but feel free to immediately open a second PR with queueMicrotask. Thanks for pushing back and making me think about it!

[divmain]

A few changes requested. Looks great so far.

[divmain]

The idea makes sense but I think it would have an undesired side-effect if multiple state atoms were updated simultaneously in a single updater. For each of the atoms that's updated by the updater, scheduledNotify would fire. And therefore, for each of the atoms, a microtask would be enqueued to notify downstream consumers. Additionally, we'd get redundant invocations of computeValue.

I made some suggestions for lazy evaluation of ComputedSignal; could you try implementing something similar for StateManagerSignal?

[divmain]

One thing I'd like to see once the lazy-compute changes are done, is to spy on doubleCount. That'll allow us to ensure that doubleCount is only invoked once after count is updated. There might be similar things worth doing for other parts of the state manager. For example, have multiple state atoms that feed into a single computed, and then make sure that computed is only invoked once when both atoms change.

[divmain]

Couple of minor things, otherwise this looks great. Please merge, no need for re-review.

[divmain]

For a follow-up work item:

We probably want to validate internalStateShape at this point. For example, we would want to confirm that Object.values(this.internalStateShape) consists only of atoms, computed, and updaters. Including other signals here might result in subtle bugs, so we should explicitly disallow that. Additionally, someone might try including something that is not signal-like and also not an updater - that should also be disallowed, as the .subscribe code below might break in that case.