Recoil icon indicating copy to clipboard operation
Recoil copied to clipboard

Add 'onSetValue' method to effect params for reactive cross-state updates

Open darthtrevino opened this issue 2 years ago • 2 comments

This PR adds an 'onSetValue' method to the arguments object passed to effect functions. This will allow atoms to subscribe to state changes in other atoms and reactively respond. This is useful for situations where atoms are dependent, but are not derivative from each other (where a selector would be useful).

Caveat: I'm not super familiar with Flow or the Recoil internals, so I'd be glad to shift anything around to match your teams' coding patterns

Fixes #1611

darthtrevino avatar Feb 15 '22 21:02 darthtrevino

Moving this comment to the top-level thread:

@drarmstr - I ran into an issue with store.subscribeToTransactions. When passing in the key of a selector. In that case it does not emit events and the observing state doesn't update. I can work around this for selectors by subscribing to the global event stream, but that is chattier than what we want, and it breaks the case when you have an atom that subscribes to another value, and you reset its state.

I put some notes in the test file and the onSetValue function. I suspect that this is a bug with the store.

darthtrevino avatar Feb 16 '22 18:02 darthtrevino

Moving this comment to the top-level thread:

@drarmstr - I ran into an issue with store.subscribeToTransactions. When passing in the key of a selector. In that case it does not emit events and the observing state doesn't update. I can work around this for selectors by subscribing to the global event stream, but that is chattier than what we want, and it breaks the case when you have an atom that subscribes to another value, and you reset its state.

I put some notes in the test file and the onSetValue function. I suspect that this is a bug with the store.

Ah, yes, that seems like a big concern. We wouldn't want to subscribe to global state changes, as that would call too often and would also be a scalability performance concern. Actually, the dirtyAtoms in the store only records atoms that have changed, not selectors, as they are derived. So, it's not really a bug in the store, but rather recording dirty selectors would be a new feature. What might work in sendEndOfBatchNotifications() would be using getDownstreamNodes() to get selectors that are potentially updated. However, I think with profiling we've seen that getDownstreamNodes() is already a performance scalability concern (See Recoil_perf-test.js) and using it there for each atom for each batch would be problematic. Options around this might be recording dirty selectors along with dirty atoms and/or optimization of getDownstreamNodes(). Issues from #1416 and #1606 are also relevant here.

drarmstr avatar Feb 18 '22 00:02 drarmstr