-
Notifications
You must be signed in to change notification settings - Fork 63
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
Let's start from the basics and work our way to what users need. #178
Comments
Update: added a way to block signals. As an alternative, one could have for (const watcher of target.#watchers) {
if (!watcher.#blocked) {
watcher.#blocked = true
const notify = watcher.#notify
notify()
}
} You could take this one step further and allow some mild memory savings by offering something close to the current API:
This would bring the API to this: declare namespace Signal {
export class State<T> {
constructor(initial: T, options: StateOptions<T>)
get(): T
set(value: T, isPending?: boolean): void
setThrow(value: unknown, isPending?: boolean): void
readonly isPending: boolean
readonly isDirty: boolean
}
export class Computed<T> {
constructor(body: () => T, options: ComputedOptions<T>)
get(): T
readonly isPending: boolean
readonly isDirty: boolean
}
export class Watcher {
constructor(notify: () => void)
watch(signal: State<any> | Computed<any>): void
unwatch(signal: State<any> | Computed<any>): void
unblock(): void
}
export function runWithoutTrackingGets<R, A extends any[]>(func: (...args: A) => R, ...args: A): R
export function introspect<T>(signal: State<T> | Computed<T>): SignalIntrospection<T>
export function currentComputed(): undefined | Computed<any>
export interface StateOptions<T> {
readonly equals?: (this: State<T>, a: T, b: T) => boolean
readonly track?: (this: State<T>) => boolean
readonly untrack?: (this: State<T>) => boolean
}
export interface ComputedOptions<T> {
readonly equals?: (this: Computed<T>, a: T, b: T) => boolean
readonly track?: (this: Computed<T>) => boolean
readonly untrack?: (this: Computed<T>) => boolean
}
export interface SignalIntrospection<T> {
value: T
isDirty: boolean
isPending: boolean
watchers: Array<Watcher>
dependencies: Array<State<any> | Computed<any>>
}
} This is just one other possible solution. |
Okay, after some further experimentation with #178 (comment), if you block the watcher right after, you can just get away with separate It'd simplify export function signalFunction(body, options=undefined) {
let ctrl
function cleanup() {
const prev = ctrl
ctrl = undefined
if (prev !== undefined) {
try {
prev.abort()
} catch (e) {
reportError(e)
}
}
}
const state = new State(undefined, {
untrack: cleanup,
})
const invoker = new Computed(() => {
cleanup()
state.isPending = true
const c = ctrl = new AbortController()
new Promise((resolve) => {
resolve(body(c.signal))
}).then(
(value) => {
if (c === ctrl) state.set(value)
},
(e) => {
if (c === ctrl) {
state.setThrow(e)
} else {
reportError(e)
}
},
)
})
return new Computed(() => {
// Register the dependency and start the body.
invoker.get()
const {value, isError} = state.get()
if (isError) throw value
return value
}, options)
} |
I don't think this is true at all. I can easily see apps with thousands of watchers. |
A lot of this seems to be based around the premise that Computed's must cache their exceptions, but I don't really see any clear argument why that's the case. We know things in JS can throw, but that's not a problem limited to signals, and we already have a solution If someone desires caching the exception then they should A) Invalidate the Computed for it's entire lifetime, drop all it's dependencies and consumers, and throw a generic broken computed error (possibly with cause). Each B) The computed passes along the exception to the caller of A is somewhat like the error caching approach, but avoids any errors inside later (past the first) watchers, since it can no longer trigger further updates. B is probably my preference though as it leaves the most freedom for error handling up to the user or libraries. Although auto-tracking makes it impossible for libs to decide whether it should be tracked before or after a possible error (just one more thing to add to my list of bad things about auto-tracking). |
@justinfagnani I said the "common" case, not the "only" case, to be clear. There is definitely no shortage of use cases for multiple watchers, and even I ran into one recently. It's just not the most common. |
@robbiespeed There's one core reason to cache exceptions generated by Let me explain this with a story. Suppose, in a web app somewhere, you're getting an expensive let worker
const result = new Signal.State()
const fetch = new Signal.Computed(() => {
worker.postMessage({type: "start", params: params.get()})
showExecutingUI()
}, {
track() {
worker = new Worker("...")
worker.onmessage = ev => {
if (ev.type === "finish") {
result.set(ev.data)
}
}
},
untrack() {
worker.terminate()
worker = undefined
},
})
const expensive = new Signal.Computed(() => {
fetch.get()
return result.get()
}) Suppose you're accessing Now, suppose Your bug report won't just be that the UI fails to update, but low-end laptop users will also complain that you're causing their laptop to heat up and slow down. You might read that and immediately check to make sure the worker is actually posting back and not getting stuck in a loop. And so you'd dive into that red herring, not knowing it's a red herring. If the exception was cached, the laptop temperature issue wouldn't happen, and simply asking users to report back their CPU usage would tell you right away that it's not a worker problem. |
There are many mistakes that can put a system in a compromised situation. class TryComputed extends Computed {
#innerComp;
constructor (compute) {
super(this.#getResult);
this.#innerComp = new Computed(() => {
try {
return { isError: false, value: compute() };
} catch (err) {
return { isError: true, value: err };
}
});
}
#getResult() {
const result = this.#innerComp.get();
if (result.isError) {
throw result.value;
}
return result.value;
}
} There are use cases where you might not desire errors to be cached. Ex: // x is not a signal and does not contain a signal, instead it has a `.isPending` which is set internally.
// y is a signal
const xPlusY = new Computed(() => {
if (x.isPending) {
throw new Error("x is not ready");
}
return x.value + y.get();
}); Caching the error would mean the computed never has any possibility to accomplish it's goal. |
@robbiespeed Your abstraction is starting to look a lot like the Also, your For other non-reactive state, if you aren't accessing something before the error, updates to it will almost certainly error again as nothing truly changed. |
Let's assume it can't be reactive, because it's from a 3rd party. If it were reactive, then throwing wouldn't be the appropriate action. The whole reason for throwing in this case is to ensure nothing gets cached, so a subsequent |
@robbiespeed I took a step back and I think I see what you're (indirectly) really hinting at: caching is strictly not necessary at all for computeds, provided there's a way to check for dirtiness. And you're almost right. Almost. Caching could almost in theory be done like this in userland: function cached(body, options) {
const equals = options?.equals ?? Object.is
let isException = false
let value
const cached = new UncachedComputed(() => {
if (cached.isDirty) {
try {
let next = body()
if (isException && !equals(value, next)) {
value = next
isException = false
}
} catch (e) {
value = e
isException = true
}
}
if (isException) throw value
return value
}, options)
return cached
} But there's a very subtpe problem: if const source = new State(0)
const isEven = new Cached(() => source.get() % 2 === 0)
const display = new Cached(() => isEven.get() ? "even" : "odd") In this, if you set In order to do this, you have to check it as part of |
No I'm not hinting that caching isn't necessary at all,
I don't believe Computed should be concerning itself with memoization, it should only care about storing the last value until it gets dirtied. Once it gets dirtied the old value should be thrown away otherwise you end up with memory inefficiencies. If people want memo they can implement that in user land themselves, and it only makes sense to do so for expensive computations. const calcXthPrime = memo((x) => /* */);
const nthPrime = new Computed(() => calcXthPrime(n.get())); |
Notifications should not block - its should be executed synchronously and cascaded as early as possible, otherwise the application will go into an inconsistent state. But reactions to these notifications should already be lazy and not run unnecessarily. I described this issue here: https://dev.to/ninjin/tonus-of-reactivity-20h1 |
Okay, I'll go back your previous comment, then:
Those should honestly not be pure signals in the first place. And in order to even drive reads to non-reactive state correctly in the first place, you have to use a messenger signal. If there's any interior branch whatsoever that could conditionally read a signal, you already need a messenger signal - it's not unique to throwing. I do see value in a signal type that always invokes its body (and necessarily // x is not a signal and does not contain a signal, instead it has a `.isPending` which is set internally.
// y is a signal
const xPlusY = new Accessor(() => {
if (x.isPending) {
throw new Error("x is not ready");
}
return x.value + y.get();
});
You can only do that if you maintain parent references. That creates significant compute overhead in This hidden complexity is something the proposal's been trying really, really hard to avoid. It always comes with both security and performance risks, and it also makes implementors question the value of it. |
There is a typical error in your analysis with the separation of signals into state and computed ones. However, you forget about lazy state calculation and automatic resource release. You are trying to solve these problems by further complicating the API by introducing class Project extends Entity {
@mems task( id: number ) {
return new Task( id )
}
destructor() {
console.log( this, 'is destroyed' )
}
}
class Account extends Entity {
@mems project( id: number ) {
return new Project( id )
}
}
class App extends Object {
@mem static account() {
return new Account()
}
@act static main() {
console.log( this.account().project(23).task(456), 'is used' )
}
static [ Symbol.toStringTag ] = 'App'
}
App.main()
// logs
// App.account().project(23).task(456) is used
// App.account().project(23) is destroyed
|
You are mistaken in thinking that watchers do not depend on signals and other watchers. If they are triggered independently of each other, then this leads to unstable behavior. Watchers should be assembled into a tree with a single root in order to have a deterministic order of calculation and ensure the correct order of dynamic (un)subscriptions in the process of updating the state graph. I described this issue here: https://dev.to/ninjin/reactive-computing-order-1ef5
|
Well, why errors and promises should be cached in the same way as the returned values, I described here: https://dev.to/ninjin/exceptions-in-reactivity-23ca
|
There are lots of different ways to implement a memoization function. You don't need to maintain a parent reference to the signal, you can pass it in to the memoized functions just like any other value. The memo internals could store the resulting value and inputs, which would automatically get GCed once the memozied function itself no longer has any references. Asides from that, there's nothing wrong with using WeakRefs. This proposal will likely need |
@nin-jin This seems to be describing a eagerly reactive graph. In a lazily reactive graph what happens after a source changes is everything it depends on ends up in what's called a dirty state. Nothing gets recomputed until it gets accessed and it's been marked dirty. At which point an error being thrown would not put the application in a unstable state, but rather it can keep everything along the path as dirty (no error cache) or mark it as an error (error cache). There are other options like "Stop" which I think is similar to what I described here as option A. Caching the error would mean that it would keep throwing the same error instance, until a source changes, at which point the computed could rerun on the next Not caching the error would mean it will rerun the computed on the next |
@nin-jin Your criticisms here would be much better suited for their own issue. Also, your The goal of this proposal anyways is to provide the underlying dependency tracking primitives, not the high-level sugar primitives. |
@robbiespeed The problem is, if I understand correctly, you'd be requiring the old computed value to be thrown away in If you're wanting to throw it away in Will note that this is technically an implementation detail and an implementor could choose to do that optimization. The two behaviors are technically indiscernible to ECMAScript code assuming |
@robbiespeed Could you bring some of this discussion of cached vs. uncached computeds to #151 if it's the same topic, and start a new issue for the error-vs-value suggestions if you consider that a separate concept? I'm not sure what you mean with the caching vs. memoization discussion but it seems like it applies equally to the main proposal and to this issue's proposed tweaked API, so I don't want it to get buried in this thread. |
This may be a naive throw-in, but the @preact/signals-core implementation uses an incrementing version counter to keep track of This could potentially act as the decider of whether to recalculate the sinks if |
Warning: this initial comment is a bit lengthy. About 2.5k words, or about a 10-15 minute read.
Synthesizing a number of issues and comments of mine, along with some discussion on Discord, into something that shows what I'm envisioning.
The current API looks like this:
That is a lot of API surface area. It's also a lot of bug surface area. And most of it could easily be managed in userland. Not to mention, the common case isn't many watchers, but one.
So I have an idea: let's go back to the drawing board and work through the basics to figure out what everyone needs. After that, let's then smooth out the edges to keep it simple, concise, and at least somewhat easy to use. Then, once we've done all that, we can add a few major nice-to-haves to improve everyone's experience.
Primitives
The most basic primitives for this model of cell-like reactive programming are the signal and the watcher.
These don't need to correlate directly with anything. Signals don't need to be objects, watchers don't need to be objects, and signal dependencies don't need to be properties of anything. Let's borrow a syntax proposed in this comment in #105 as pseudo-code to show how each of these work out.
signal direct = value
signal computed = direct
value = direct
value = computed
direct = value
computed = value/direct
The current API can only capture some of this:
direct = new State(value)
computed = new Computed(() => direct.get())
value = direct.get()
value = computed.get()
direct.set(value)
There's a neat little trick here that could be exploited to recover that missing piece, though: store the body in a
Signal.State
and just read and invoke it on every update. Here's how one could do it with the current API:Turns out, if you want to round out that table, all you need to do is instead use that relatively simple
WritableComputed
wrapper class:direct = new State(value)
computed = new WritableComputed(() => direct.get())
value = direct.get()
value = computed.get()
direct.set(value)
computed.set(() => value/direct.get())
So clearly, that's not a true primitive.
We do of course still need to be able to watch signals. We could just model that as
signal.watch(notify)
to add,signal.unwatch(notify)
to remove. Andnotify()
would be called each time a signal becomes dirty (read: whenever the watched signal or any signal it depends on is.set()
).To preserve batching, signals need to suppress invoking their notifier until instructed otherwise. One might think a way to do this is to do one-shot watchers, but that'll interfere with potential "on tracked"/"on untracked" hooks. So it'd be easier to just add a
signal.unblock(notify)
instead.So far, we've found we need the following:
state = new State(initial)
computed = new Computed(body)
value = signal.get()
state.set(value)
signal.watch(notify)
signal.unwatch(notify)
signal.unblock(notify)
Exceptions and hooks
Now that we know what our base object types are and our most primitive methods, let's go over another elephant in the room: exceptions. Unfortunately, most things can throw them. Even in the language, they're everywhere. Even most expressions can throw them if you don't first coerce their operands (and the only coercion that can't throw is boolean coercion).
So now we have to add that in. One easy way is to just have
Computed
cache its exception. Easy enough, right?Nope. Let's look at a simple throttling combinator that, if a call is throttled, updates one last time after the throttle duration expires so it doesn't lose any important updates.
This looks simple enough. But it has a neat little pitfall: imagine if
signal
isnew Computed(() => { throw new Error("wat") })
. Do we cache the error? If you say yes, imagine if your caller does this:Suddenly, you're spamming
/notify
over 10 times a second. And almost certainly you'll be encountering frame rate issues. Definitely not a good user experience.So yep, we have to capture that error:
Of course, that's easy enough with that mostly-sync signal. Doesn't require explicit API surface, just a
Computed
that caches exceptions.But we're all set now, right? Nope, we're still potentially invoking timers that might not even get used. Let's save whatever watcher some trouble and clean up after ourselves. We'll need a hook letting us know when we no longer have anyone watching us. Let's call that
untrack
.Async and Streaming
Suppose we want to convert an
EventSource
into a signal. The API is simple to wrap. There's three events,open
,message
, anderror
, and there's no extra properties to fuss with.Thing is, this is a bit too eager. Let's make this lazier. A lot lazier. We might not need the event source right away, so why should we create one? Also, suppose we start watching the event source, stop watching it, and then start watching it again, all from the same signal. If that happens, we now have a disconnected source that isn't reconnecting. Let's instead allocate it on demand, so everything "just" works. And in order to do that, we need a
track
hook.What about that
signalFunction
from earlier. How would we implement that? Once again, we need exceptions.Simple enough. Didn't need much. But we have a problem: how do we know data is ready? Imagine we have code like this using it:
Now, suppose we were just told to show a loading spinner modal while we're waiting. Unfortunately, this means we need something to tell us out of band when our data is meaningfully "ready". And the simplest way? Throw a
.isPending
to tell us we're still waiting on the data to come.That's all fine and dandy, but how do we get that? Well, we need two things:
So, how about we augment
state.set
to accept an extraisPending
option. Now it looks likestate.set(value, {isPending})
, whereisPending
defaults tofalse
(as most things are sync and we aren't waiting on them). And let's haveComputed
propagate that flag from its dependencies. So the abovestate.isPending
works,signal.isPending
will also need to be auto-tracked similarly tostate.get
.Now, we can fix
signalFunction
to reflect that state.Computed
optionsYou've probably noticed me passing an options object to the
Computed
constructor. Turns out you can do all of that exceptequals
in a helperState
+Watcher
.Unfortunately,
equals
cannot be desugared so easily. I'll leave it as an academic exercise, but you'll need a watcher as well as a state variable and just some generally awkward coding to avoid accidentally linking the state variable to that inner watcher. But suffice to say, while you don't technically needequals
forComputed
, it'll be way simpler to implement in that method.Introspection
Sometimes, you want to be able to optimize a data flow. If nothing changed, why bother re-getting the value at all. This comes up a fair bit with things like virtual DOM trees: if you have a signal returning a tree, you can save considerable rendering time by checking if the signal is "dirty" (read: has an update that hasn't been read yet) and skipping the whole process if it's not. This necessitates a new property on both computed and state signals:
signal.isDirty
.Now, suppose you want to know what watchers are watching a given signal. Simplest way to expose that is via a simple function:
watchers = signal.getWatchers()
. You can also use this to figure out which signals from an array are connected to a given watcher.In some niche use cases, it may also be useful to know what signals a given signal depends on. This can be provided very simply via
deps = signal.getDependencies()
.In some niche use cases, it may be helpful to know what the current
Computed
context is. This can be provided very simply viacurrentComputed()
.Variadic watchers
The current
Watcher
class could be simulated usingwatch
,unwatch
, andisDirty
. It doesn't even need any of the rest of the introspection.Real core
So the real core now looks like this, where
signal
can be eitherstate
orcomputed
:state = new State(initial, options?)
options.equals(a, b)
options.track()
options.untrack()
computed = new Computed(body, options?)
options.equals(a, b)
state.set(value, {isPending?})
value = signal.get()
signal.isPending
signal.isDirty
signal.watch(notify)
signal.unwatch(notify)
signal.unblock(notify)
runWithoutTrackingGets(body)
watchers = signal.getWatchers()
deps = computed.getDependencies()
API Refinement
State
s are simple, and pretty much everything is needed:state = new Signal.State(initial, options?)
Options:
options.equals(a, b)
options.track()
options.untrack()
Edit: they apparently do have a case where the track/untrack options come super in handy so they can just receive all the same options. (h/t @shaylew)Computed
s don't have much (if any) use case for options other thanequals
. So I'm dropping them. It's otherwise very similar.computed = new Signal.Computed(body, options?)
And for
.set
, an object is rather expensive in a path potentially that hot. Let's lower that to an inline boolean.state.set(value, isPending=false)
You may have noticed this pattern a lot. And I mean a lot.
Let's optimize this by adding a
.setThrow(e, isPending=false)
that stores an exception to later throw from.get()
. It'll be stored the same way as it is inComputed
. This also simplifies the graph walk a bit and letsComputed
andState
share most their.get()
implementations. And while we're at it, let's make.isPending
writable, since it's sometimes useful to explicitly set it without also changing the value. (If both need changed separately, it's commonly batched anyways.)To see this in action, here's `signalFunction` re-implemented in terms of that.
It also makes the `EventSource` helper a bit more readable.
The watchers are functionally the same for either type, so those make more sense as static methods.
Signal.watch(signal, notify)
Signal.unwatch(signal, notify)
Signal.unblock(signal, notify)
For the introspection methods,
signal.getWatchers()
andcomputed.getDependencies()
, it makes more sense to just combine the two methods into a singleSignal.introspect(signal)
call and just throw everything in there. If more things turn up that's interesting, it's easy to extend it later. You could even extend it with other existing fields like.isDirty
,.isPending
, and.value
. So let's just wrap all that into just that one convenience method.info = Signal.introspect(signal)
For
runWithoutTrackingGets
, let's give it the ability to forward arguments. Once natively implemented, engines can optimize for that and inline away the boilerplate, much like what they already do forReflect.apply
andFunction.prototype.call
. Engines may even be able to detect the idiomSignal.runWithoutTrackingGets(() => ...)
and elide the closure in JIT code.result = Signal.runWithoutTrackingGets(func, ...args)
The rest is fine as it is.
value = signal.get()
signal.isPending
signal.isDirty
New type definitions
This is what would be exposed at the language level:
Possible questions
I found a typo or mistake.
Let me know and I'll fix it as soon as I can.
What exactly are you trying to accomplish here?
My goal is to identify the minimal core and re-focus the discussion to center around that. A clean sheet base design, if you will.
Even if my particular design isn't chosen, it'd be a win in my book to just steer discussion away from various features and to just focus on the core concepts and how they would be helpful.
A simple API is a secure and performant API, so I'm all for applying the KISS principle to this. Simplicity is especially important for an API attempting to be present as a low-level support API and not just a high-level userland API.
What happened to
subtle
? Why are we throwing it away?The analogy to
crypto.subtle
is tenuous at best.As stated there, the reason it's
crypto.subtle
and not justcrypto
is because it truly is subtle. There's an entire class of flaws that could show up in code usingcrypto.subtle
that won't show as errors. They won't show as anything. No number of unit tests will help you find them. And when you do fix them, you often can't write a meaningful regression test to make sure it doesn't happen again. Each change to code using it requires thorough understanding of both the code and the mathematical underpinnings of the surrounding system's design. If a test vector doesn't yield the expected result, but the code otherwise executes successfully, step debuggers are frequently useless. These kinds of subtle yet extremely dangerous flaws are precisely why the API's exposed ascrypto.subtle
and not justcrypto
.This is very much not the case for signals. Yes, you could run yourself into a very difficult time debugging if you're not careful with your states. But you will not have issues with step debuggers. And you will not be dealing with bugs you cannot write regression tests for.
Oh, and there is some precedent already for sharp tools not existing in special namespaces, even among newer additions. To name a few big ones:
Proxy.revocable
doesn't live in a special namespace.getPrototypeOf
being potentially very large footguns.FinalizationRegistry
is a very tough API to use correctly. You can write automated tests to test stuff using it, but such tests would be inherently very flaky. And it's extremely easy to accidentally retain a strong reference to the source object - it takes special object modeling to not do so. If anything, thecrypto.subtle
analogy fits this far better than anythingSignal
-related.Why
runWithoutTrackingGets(...)
and notuntrack(...)
?That name really needs bikeshedding. And
untrack
doesn't quite encapsulate what the function really does.Why toss out the
Watcher
class?Three reasons:
Computed
in practice.Computed
one, do what you would normally do but in thatComputed
's body, and call it a day.Isn't this a lot like railway-oriented programming?
Kinda yes, but without the explicit combinators.
Isn't this a lot like $LIBRARY/FRAMEWORK?
Probably.
I'm admittedly not the most well-versed in the world of signals. I come from a deep background of React-like frameworks, being a former maintainer of a well-known React competitor (I'm still semi-active behind the scenes in that project, just not in GitHub directly). Until about a week ago, I myself was rather skeptical of the value of signals. I'm still learning the lay of the land of signals here, so please bear that in mind!
My question isn't answered here. What about $X?
Well...lemme grab my crystal ball real quick. $ANSWER.
In all seriousness, feel free to ask me anything. I'm all ears, and as I said in the last question, I'm still learning. I'm certainly not the signals expert here, so I'm pretty much expecting to have missed something significant.
I'm just a nobody. I feel my question might be stupid or a complete and utter waste of time.
No it isn't. And I'm gonna be honest: if you're afraid to ask, you're the kind of person I want to hear the most from. I'm not joking. Don't be afraid - I won't bite. And neither will the rest of us here. 🙂
cc @littledan for visibility
The text was updated successfully, but these errors were encountered: