Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convenience methods #32

Open
littledan opened this issue Oct 23, 2023 · 24 comments
Open

Convenience methods #32

littledan opened this issue Oct 23, 2023 · 24 comments

Comments

@littledan
Copy link
Member

Should the Signal API include any convenience methods? Possibilities:

  • Signal.isSignal -- brand check
  • Signal.State.prototype.update -- read/run/write
  • Signal.State.prototype.toReadonly -- make a computed signal out of a state signal, to drop the capability to write to it
  • A version of Signal.Effect that schedules itself (badly)

Would any other convenience methods be useful? The current API omits all of these on purpose, with the goal of being minimal.

@benlesh
Copy link
Collaborator

benlesh commented Dec 26, 2023

From another discussion: There's a question about whether or not we should have a way to read a value if it's a signal or a value... or make sure it's a signal first etc. Similar to Promise.resolve(promiseOrValue) should there be a Signal.of(signalOrValue) or Signal.read(signalOrValue)?

The use case would be something like a component that accepted a property that could either be a signal or a plain value. Let's say the author wants to encourage the use of signals for managing dynamically changing state, but the state doesn't always need to dynamically change. For example with a "layout component".

@NullVoxPopuli
Copy link
Collaborator

potentially also, localCopy, which allows state-forking, but re-sets itself with the upstream value if that changes.

Demo / behaviors here:

(some context: https://twitter.com/nullvoxpopuli/status/1770862533378707571 )

@jods4
Copy link

jods4 commented Apr 1, 2024

peek has been there since early KO and is a simple, more efficient untrack when reading a single Signal/Computed.

signal.peek() and computed.peek() behave like .get() does when called outside a computing context, i.e. they return the current value without tracking the read.

@EisenbergEffect
Copy link
Collaborator

I think wee need untrack() because there are scenarios when an entire block of operations needs to be performed and where the code doesn't have direct access to the signals. But, it might be that peek could be added as an additional API.

@justinfagnani
Copy link
Collaborator

I have a utility that deals in a single signal and needs to peek it. Having a Signal.subtle.peek(signal) function would be great for that.

@WebReflection
Copy link

WebReflection commented Apr 2, 2024

FWIWI I've resolved the peek() case as such and it seems to work well:

class State extends Signal.State {
  peek() {
    return untrack(() => this.get());
  }
}

The isSignal also seems easy to implement via a couple of instanceof operations but it might poison arbitrary libraries built on top of this API so I think we're better off without it (i.e. I am not extending Signal.State but I am returning signals and these should not fail expectations).

If effect and batch would be in somehow in a least convoluted, yet effective, way, this API would be overly awesome!

@jods4
Copy link

jods4 commented Apr 2, 2024

@EisenbergEffect Yes, I wasn't suggesting that peek replaces untrack. A way to completely escape a computing context is required (see effectScope discussions in Vue for example).

But sometimes all you need is reading one signal and untrack(() => this.get()) is not great DX, nor very efficient compared to peek implementation.

@WebReflection
Copy link

WebReflection commented Apr 2, 2024

@jods4 untrack is not about just one signal or computed, it's about running something potentially huge ... peek() as it is in Preact is for each single signal/computed and it's an explicit user intent to not track that particular change.

Preact has untrack too so there is also previous work around this difference which is very important.

Accordingly, I wouldn't mix the two use-cases or needs, untrack is fine as it is and it can be used to abstract peek already, peek as State or Computed method might be welcome but it's a different use case than untrack: peek is atomic, untrack doesn't need you to change code around a signal so I hope we cna either have both or just have untrack and provide that abstraction like I did already so that users can explicitly peek single values or untrack callbacks with potentially dozen signals in it.

edit also let's keep in mind this API proposal is for libraries authors ... the DX argument is hence weak as long as the desired result is achievable and untrack allows peek to exist, if a library author wants to provide that ability ... if we go in features-creep-land this API won't ever reach the Web, which would be unfortunate, as it's already awesome as it is and it has it all behind the scene.

@jods4
Copy link

jods4 commented Apr 2, 2024

@WebReflection that's exactly what I said, not sure how you understood my previous messages.
I said untrack is required and cited an example why in the context of Vue.

the DX argument is hence weak as long as the desired result is achievable and untrack allows peek to exist, if a library author wants to provide that ability ... if we go in features-creep-land this API won't ever reach the Web, which would be unfortunate, as it's already awesome as it is and it has it all behind the scene.

Yes, anyone can build peek as untrack(() => signal.get()).
Doing so they create a closure and call a function that has to change the computing context using a stack, before calling that closure in a try/catch/finally block.

On the other hand, built-in peek on signal is basically peek() { return this.#value }.

I'm not sure that's a lot of feature creep, especially considering that most (all?) "signal" implementation for the past 10 years have exposed a peek in their core set of features.

@WebReflection
Copy link

checking internals of the polyfill, it could indeed just return the internal symbol value and avoid extra stacks. Apologies I've misread your previous comment, I've thought you preferred peek() over untrack(), my bad. Agreed having both would be nice.

@EisenbergEffect
Copy link
Collaborator

Great conversation here! Thanks for the clarification @WebReflection and @jods4!
QQ: If we have peek, is there any use case for a poke as well, to set the value?

@WebReflection
Copy link

WebReflection commented Apr 2, 2024

@EisenbergEffect I feel like that's a stretch too far ... or better, I don't understand the use case for it, it feels surprise prone to me as "a sneaky update nobody should notice" and that to me won't lead to better code ... .peek() is an explicit opt-out to subscription, .poke(value) as opt-out to reactivity in general feels odd (imho).

if we take previous art work around the topic, I am not sure which library exposes that to date but if they do, it's also easily an attack vector to me if these libraries land in undesired places where untrusted code starts poking around foreign code ... it's chaos easy to generate, while .peek() remains into the "observable field", if that makes sense (and the code owner can trust).


edit to some extend, think .poke(value) as an easy way to dispatchEvent nobody can listen to, and .peek() as an easy way to just get .value ... the former has catastrophic effects if not well orchestrated accordingly, the latest is always harm-free for the rest of the logic.

@jods4
Copy link

jods4 commented Apr 2, 2024

@EisenbergEffect RE poke:
In all these years I don't think I've ever encountered a use-case for setting a value without triggering signal.
It feels dangerous because all dependencies think they're clean although they're not.

"Triggering" computeds or bindings even though they "have not changed" is a topic that comes up quite often in UI frameworks. It can often be worked around with better designs or by introducing missing signals (even just a counter that's incremented when a refresh is desired).

@WebReflection
Copy link

agreed and on a second though, untrack is fine to exist and has tons of use cases already, .peek() is the "atomic untrack" within a state/computed and it's fine and has tons of use cases ... .poke(...) is dirty by design and doesn't even have its own untrack like counterpart ... I would discourage exploring that field as it can't produce anything good to me, not even libraries authors ... if it lands a .poke(...) it should land with those funny comments like "YOU_ARE_FIRED_IF_YOU_INVOKE_THIS" and while I am sure there is a single edge case that makes sense in the world about it (such as, update an object reference underneat with same shape but different reference for the GC) I think is disaster prone if exposed and, once exposed, of course libraries authors will use it then ... please let's drop this idea already 😅

@NullVoxPopuli
Copy link
Collaborator

untrack is fine to exist and has tons of use cases already

do we have a list of these? would be good for a "guidance" and "recommendations" section of the docs / proposal, because there are a lot of ways to shoot one's self in the foot with reactive systems.

May be of interest:

@EisenbergEffect
Copy link
Collaborator

Thanks everyone! I was pretty sure poke was a bad idea and I couldn’t think of any use cases, but wanted to make sure I wasn’t missing something.

@WebReflection
Copy link

@NullVoxPopuli

do we have a list of these?

just the fact one can send errors or debug on Promise catch would be enough to me, as you never want to subscribe to those callbacks

fetch(remote.get()).catch(error => {
  untrack(() => {
    console.error(`Unable to fetch ${remote.get()}`, error);
  });
});

Sure thing for that specific use case .peek() would be even better but with callbacks around used to effect and compute having a way to introspect their values/invokes without subscribing is desirable.

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Apr 3, 2024

I don't think callbacks like that are inherently subscribed to, and untrack wouldn't be needed 🤔 i am about to verify this tho in
proposal-signals/signal-utils#19

More info soon!

@littledan
Copy link
Member Author

Yeah my feeling is that events/microtask queue items would have a null currentComputed by default, and just not be tracking until you go into a Computed.

@WebReflection
Copy link

WebReflection commented Apr 3, 2024

my example was poorly presented ... I meant that whenever signals or computed are around and you want to:

  • serialize that value to send a JSON request to a REST API
  • react to (caught) errors that might be sync or not and store or send involved signals in there
  • introspect anything due debug flag enabled within the code, and maybe transpiled away in production
  • add A/B tests around features that should not compromise the user logic when enabled ...
  • stuff like this ...

I can do a quick search in Preact repo though as I am sure they had various reasons/requests/discussions aroud this topic, if that helps anyone (or users wanting to understand when untrack and peek() are desirable)

To my DOM examples based on signals, there are cases I need to retrieve a value in sync with the rest of the flow but without subscribing the whole thing to my introspection logic, so this is another use case ... libraries' authors will need, and want, a way to not subscribe for foreign data they don't understand where it comes from or how frequently it could update.

edit ... I mean, without going too far from my own repositories I recently had complains about untracked too so I guess people use these features daily: WebReflection/signal#6

More requests around untrack here: WebReflection/usignal#18

This is still my own stuff, which was fully inspired by Preact signals/core

@jods4
Copy link

jods4 commented Apr 3, 2024

If you're fishing for peek / untrack use cases:

Very recently I wanted to capture the initial state of reactive objects, to be able to provide a "dirty" flag. This happened in a computed that "initialised" the transactional object from the currently bound object.
It's easy to understand that this computed should only be dependent on the bound object instance (if instance changes, a new transactional object is created).
Because this step read all reactive object properties to capture initial values, they became dependencies and a new transactional object would be created at each change, which is completely broken.
My solution: peek at reactive properties when capturing initial values, so that those reads are not reactive (it's intuitive as well: by definition initial values never change).

You can also dig into Vue effectScope RFC: vuejs/rfcs#212
It's a broader API mainly targeted at ownership / lifetime outside of components, but one motivation for the design was to be able to escape the current computing context, see Detached Nested Scope.

Vue also exposes pauseTracking() and resumeTracking() as advanced apis, that serve an identical function as untrack().

I previously mentionned that Knockout has had peek since forever: https://knockoutjs.com/documentation/computed-dependency-tracking.html#controlling-dependencies-using-peek. The doc gives as an example a computed that makes an AJAX request based on a page number, while also keeping track of the selected item although the latter is not a depedency (no AJAX request if selected item changes).

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Apr 7, 2024

Here is a userland implementation of peek (via localCopy without an effect or untrack):

@EisenbergEffect
Copy link
Collaborator

EisenbergEffect commented Apr 12, 2024

@littledan We've had the request for peek() come up in several different conversations and there's a lot of precedent for it in existing libraries. Since one of our goals is performance/memory-efficiency, this seems like a reasonable thing to add to eliminate callback/closure usage in a number of common scenarios. What do you think about adding this to both State and Computed?

@WebReflection
Copy link

WebReflection commented Apr 12, 2024

my 2c: explicit peek() is intentional zero side-effects by design so I hope we can make it part of the specs ... poke(value) as suggested elsewhere as counter-method is undesired side-effects by design so it should likely never land in these specs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants