LegendApp / legend-state

Legend-State is a super fast and powerful state library that enables fine-grained reactivity and easy automatic persistence
https://legendapp.com/open-source/state/
MIT License
2.86k stars 81 forks source link

observe() called twice when linked observable is accessed #313

Open dbrelovsky opened 3 months ago

dbrelovsky commented 3 months ago

If a linked observable is accessed in observe(), the callback is executed twice.

it('should be called only once', () => {
  const data$ = observable(1)

  const obs$ = observable({
    a: 1,
    b: () => data$
  })

  let observed = 0

  observe(() => {
    observed++

    obs$.b.get()
  })

  expect(observed).toBe(1) // AssertionError: expected 2 to be 1

  data$.set(2)

  expect(observed).toBe(2) // AssertionError: expected 4 to be 2
})
jmeistrich commented 3 months ago

It's due to the way that sets cascade through links, and it observes both the source and target to make sure it catches everything. So it observes the target (data$) changing and re-runs, and the same batch triggers obs$.b to re-run, which triggers the observer again. Better to observe too many times than miss it!

I think we can do better, but it will take a refactor of the way that listeners are notified. My current theory is to notify in waves of priority, where computed functions would have a high priority and observers would have a lower priority. Then the observe would be triggered once by both the target and computed changing. I hadn't planned for this linking functionality when I initially designed the notification system so it's not totally ideal for that scenario.

It will probably be a little bit until that project becomes a priority, although the optimizing instinct in me really wants to do it now 😅. I may end up tackling it on a long plane or train ride...

But I don't think should should be breaking anything or causing any real problems for now. But please let me know if you see any really bad side effects and we should prioritize it higher.