tc39 / proposal-signals

A proposal to add signals to JavaScript.
MIT License
2.95k stars 54 forks source link

remove discouragement of `untrack` #89

Closed titoBouzout closed 2 months ago

titoBouzout commented 2 months ago

See https://github.com/proposal-signals/proposal-signals/issues/52

The total point of the API is not to cause subscribers, and to not re-run when the signal change, for performance or any other reason.

NullVoxPopuli commented 2 months ago

this may be my opinion, but untrack can also cause later-logical and general reactivity issues in later iterations of rendering in UI -- it's def a power-user utility, and is a sharp tool that must be used with care

titoBouzout commented 2 months ago

I understand your concern, but I think that should be on its own issue, as it's a different topic. Maybe a good idea is to start a discussion to identify the problematic uses of signals, like writing to signals in an effect, reading and writing to the same signal in a line such signal.set(signal.get()+1), etc. From there, propose solutions, instead of marking APIs as unsafe, because by that logic, effect is unsafe and writing to a signal too.

EisenbergEffect commented 2 months ago

@titoBouzout I don't recall seeing someone mention this as an issue before, so can you expand on the problem with this?

signal.set(signal.get() + 1);

How would you recommend someone increment the value?

titoBouzout commented 2 months ago

@titoBouzout I don't recall seeing someone mention this as an issue before, so can you expand on the problem with this?

signal.set(signal.get() + 1);

How would you recommend someone increment the value?

The problem with that snippet is that signal.get causes tracking, and signal.set changes the very same tracked value, if you do that in a computed/effect, it will loop forever. It's an antipattern that shouldn't be used anywhere, not even in untracked places because people see these snippets and then copy and paste into computed/effects.

Solid solves this by providing the signal.set(prev => prev + 1); form, which is setting a signal using the previous value without causing tracking when giving to the callback the previous value.

EisenbergEffect commented 2 months ago

Hmm. I think you may be making some assumptions based on a particular implementation you are familiar with. This code does not cause an infinite loop when used in a Signal.Computed and it doesn't cause an infinite loop when used in the sample effect from the polyfill readme either. It's possible someone could write an effect that does have that problem. So, some guidance or "things to take into consideration" should definitely be added. Note that the polyfill readme points out that users should understand the constraints of their chose effect system.

titoBouzout commented 2 months ago

Hmm. I think you may be making some assumptions based on a particular implementation you are familiar with. This code does not cause an infinite loop when used in a Signal.Computed and it doesn't cause an infinite loop when used in the sample effect from the polyfill readme either. It's possible someone could write an effect that does have that problem. So, some guidance or "things to take into consideration" should definitely be added. Note that the polyfill readme points out that users should understand the constraints of their chose effect system.

Then the implementation is broken. That's not how signals work.

NullVoxPopuli commented 2 months ago

That's not how signals work.

I mean, that's why we're here, to define how they work in a way that allows for easier and more ergonomic authoring, attempting to solve all the problems encountered by all variants of this type of reactivity over the last good many years <3

We can fix things! 🎉

titoBouzout commented 2 months ago

Yeah, and I am trying to help :) I am a heavy user of signals.

How comes reading to a signal and writing to it on a tracked place doesn't cause the tracked place to rerun for eternity? What if you want to do just that but cut the loop when the counter reaches 10? I honestly don't see how the implementation could be right if doesn't re-run.

I didn't mean to sound rude or discouraging, but if something looks broken to me, I would just say it.

EisenbergEffect commented 2 months ago

I'm not an expert on this area of the proposal. I just wanted to point out how it works today. I wasn't involved with the conversations around this part of the spec, but I do know that Ryan has been involved and Milo has been extremely active.

@modderme123 Do you want to jump in and clarify the above behavior?

littledan commented 2 months ago

This thread forms a pretty strong argument to add an .update(oldValue => newValue) method to Signal.State--a case where untracked is valid/sound. PRs welcome.

NullVoxPopuli commented 2 months ago

Personally, I'd like to see some examples <3 I've seen some similar conversation here, https://github.com/proposal-signals/proposal-signals/issues/92#issuecomment-2029115147 and provided some counter-examples of where .update(old => new) wouldn't be needed -- but maybe it's simply meant to be an ergonomics shortcut? if so, no big deal! it makes sense to have, but I don't think it should behave any different from s.set(s.get() + 1) (I attempted to explain my feelings on this in the linked comment there) :sweat_smile:

titoBouzout commented 2 months ago

Is not ergonomics, is about updating a signal value without causing tracking.

Anyway, we are getting sidetracked. Can we merge this PR and talk about the issues in an organized way? I opened this issue for the updating a signal value without causing tracking https://github.com/proposal-signals/proposal-signals/issues/92 we can open another issue for the issues you are seeing with untrack

littledan commented 2 months ago

Let's continue discussion in #92 for now.