preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.64k stars 89 forks source link

unwanted "effect" invocations #416

Closed rozek closed 9 months ago

rozek commented 10 months ago

Consider the following code

  import { signal, effect } from '@preact/signals'

  let a = signal(0)
  let b = signal(1)
  let common = signal(0)

  effect(() => {console.log('- a from c',common.value); if (a.value !== common.value) { a.value = common.value }})
  effect(() => {console.log('- c from a',a.value); if (common.value !== a.value) { common.value = a.value }})

  effect(() => {console.log('- b from c',common.value*2); if (b.value !== common.value*2) { b.value = common.value*2 }})
  effect(() => {console.log('- c from b',b.value/2); if (common.value !== b.value/2) { common.value = b.value/2 }})

  console.log('a is now',a.value,'b is now',b.value)

  console.log('\nsetting a to 1\n')
    a.value = 1
  console.log('a is now',a.value,'b is now',b.value)

  console.log('\nsetting b to 3\n')
    b.value = 3
  console.log('a is now',a.value,'b is now',b.value)

  console.log('\nsetting common to 2\n')
    common.value = 2
  console.log('a is now',a.value,'b is now',b.value)

This code logs

- a from c 0
- c from a 0
- b from c 0
- b from c 0
- c from b 0
a is now 0 b is now 0

setting a to 1
- a from c 0
- c from a 0
- a from c 0
a is now 0 b is now 0

setting b to 3
- b from c 0
- c from b 0
- b from c 0
a is now 0 b is now 0

setting common to 2
- a from c 2
- c from a 2
- b from c 4
- c from b 2
- b from c 4
- a from c 2
a is now 2 b is now 4

which is wrong (or, at least, not what was intended).

The underlying problem is that code such as

effect(() => {if (a.value !== common.value) { a.value = common.value }})

which is intended to be run when common.value changes, also gets triggered when a.value changes - and then changes a.value again. It would be easier if one could exclude a.value from being tracked in this code.

Of course, one could peek into a, but - in my real app - code like the one shown here is intended to come from a plugin - and plugin programmers should not have to know whether a value they access is observed or not. Right now, they would even have to write two versions of their code: one for observed and one for unobserved values...

Simply untracking the plugin code destroys the effect.

Does anybody have any idea?

rozek commented 10 months ago

right now, my only idea is to simply remove the condition if (a.value !== common.value) and rely on proper signaling when common.value has changed...

rschristian commented 10 months ago

The underlying problem is that code such as

effect(() => {if (a.value !== common.value) { a.value = common.value }})

which is intended to be run when common.value changes, also gets triggered when a.value changes - and then changes a.value again. It would be easier if one could exclude a.value from being tracked in this code.

This is fundamentally how the API works -- if you only intend for the effect to run when common.value changes you need to avoid tracking with peek.

Of course, one could peek into a, but - in my real app - code like the one shown here is intended to come from a plugin - and plugin programmers should not have to know whether a value they access is observed or not.

Sure, but they should be using peek defensively to solve this. Don't subscribe to signals you don't need to.

Right now, they would even have to write two versions of their code: one for observed and one for unobserved values...

I'm not quite sure I understand the real world problem here. Keeping signals in sync like this is almost certainly the wrong API design, with better ways to go about it (like passing the original signal itself into whatever kind of plugin this is).

rozek commented 10 months ago

Well, it may be "how the API works", but this leads to the same desaster as the horrible "async" handling in JavaScript (where every user of a function has to know whether its sync or async and behave accordingly - and implementations must not switch between both.)

Here, every "user" (i.e. effect callback) has to know which values are observed and which are not - libraries such as deepSignal (which remove the need for .value) become useless as users still have to know the implementation details

rschristian commented 10 months ago

Here, every "user" (i.e. effect callback) has to know which values are observed and which are not

I'm not sure I follow; why would an effect callback not know which values it was meant to observe? Perhaps I'm misunderstanding what you're after, but this is a rather core piece of the design.

libraries such as deepSignal (which remove the need for .value) become useless as users still have to know the implementation details

The value proposition of add on libraries is for their maintainers to vouch for, but are you referring to the fact that a consumer might not be aware of whether it is passed a signal or a plain value? If so, TS types & instanceof Signal will help a lot here. It might be a bit more verbose, but you could do something like this:

import { signal, effect, Signal } from '@preact/signals';

const count = signal(0);

// Pretend module boundary

const signalMaybeUnwrapper = (maybeSignal) =>
    maybeSignal instanceof Signal
      ? maybeSignal.peek()
      : maybeSignal;

effect(() => {
    if (signalMaybeUnwrapper(count) !== common.value) {
        ...
    }
});
rschristian commented 9 months ago

Going to close this out, feel free to reply if you still have questions & we can reopen.

rozek commented 9 months ago

oh, I must somehow missed your last post, sorry for that. The TS example is indeed a nice idea!

Thank you very much for your effort! Yes, you may close the issue to keep records clean