-
Notifications
You must be signed in to change notification settings - Fork 58
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
Watcher simplification #222
Comments
Here's how sync effects could be implemented using this and the DOM's function effect(token, body) {
if (token.aborted) return
const signal = new Signal.Computed(body)
Promise.resolve().then(function loop() {
if (token.aborted) return
Signal.subtle.waitForUpdate(signal, token).then(loop)
signal.get()
})
} |
I find this pretty intriguing. If the performance were as good as the watcher approach, this would be simpler to manage. @littledan @alxhub What do you all think of this idea? |
Edit: clarity, hide long quote Will note that this algorithm also gives a way to solve a concern I highlighted in #195 (comment) about the existing Important bits copied here
With this suggestion, to ensure proper subscribe/unsubscribe batching across multiple disjoint signals, you'd just do the following:
|
Oops, forgot to cite specific precedent here to explain my "I'm pulling a page out of embedded and low-level systems development with this one" statement in my initial issue comment:
|
As for performance, I don't have benchmarks, but suspect it to be margin of error. It does preclude refreshing inside the same job (required for same-animation-frame scheduling), but I doubt it'd be a problem in practice when you can just check if it's dirty again to know right away, and re-getting the value synchronously will cause the queued |
I think the forced asynchronous nature of this API would be a problem for some use cases. Right now you can respond to changes sooner than a microtask as long as the notification callback has returned. I also worry about the number of Promise allocations in cases where you have a lot of signals that you want to watch continuously and independently. Watchers are at least persistent already, and I'm hoping that we could consolidate watchers if we get notifiedBy. I wonder how many watcher use cases are even one-shot? Is that the exception? |
@justinfagnani I did list one case where it could bring a material limitation, but I doubt it'd be much of one in practice - it's bad practice to do same-frame re-rendering like that in most cases anyways and most frameworks just don't support it. Did you have any others in mind? I'd love to hear. (I'm always happy to be proven wrong about my intuition.)
I considered a
Most aren't, but IMHO it's a bit easier to reason about the state truly needed if you at least start from the one-shot angle (even if you don't end there). |
Angular depends on the faster-than-microtask / semi-synchronous watch behavior as well. In general:
|
@alxhub Would a Edit: or a Edit 2: used this as justification to (again) push for synchronous promose state inspection: https://es.discourse.group/t/synchronous-promise-inspection/2037 |
No, |
Okay, then on that note, I'll rescind my
So, the current design plus my class Watcher {
constructor(notify) {
this._notify = notify
this._phase = "ready"
this._signals = new Set()
this._newlyAdded = new Set()
this._newlyRemoved = new Set()
}
watch(signal) {
if (this._phase === "notify") throw new TypeError()
if (this._signals.has(signal)) return
this._signals.add(signal)
this._newlyAdded.add(signal)
this._newlyRemoved.delete(signal)
maybeAddWatchedParentToChildren(signal)
}
unwatch(signal) {
if (this._phase === "notify") throw new TypeError()
if (this._signals.has(signal)) return
this._signals.add(signal)
this._newlyAdded.delete(signal)
this._newlyRemoved.add(signal)
maybeRemoveWatchedParentFromChildren(signal)
}
evaluateAndMarkReady() {
if (this._phase === "notify") throw new TypeError()
const added = this._newlyAdded
const removed = this._newlyRemoved
this._newlyAdded = new Set()
this._newlyRemoved = new Set()
this._phase = "ready"
for (const signal of added) signal.options?.tracked?.()
for (const signal of removed) signal.options?.untracked?.()
}
}
function notifyWatcherOfSignalSet(watcher) {
if (watcher._phase !== "ready") return
watcher._phase = "notify"
try {
watcher._notify()
} finally {
watcher._phase = "pending"
}
} |
I'll go ahead and close this. My concern was addressed. |
Have you considered one-shot notification for watchers, pushing the managed
Watcher
object itself to userland?I'm thinking something like this: a promise that resolves as soon as a signal's made dirty.
untrack
isn't called prematurely.promise = Signal.subtle.waitForUpdate(signal, token?)
resolves as soon as the signal becomes outdated again.track
hook called synchronously. If it throws, the error is reported similarly to uncaught rejections.equals
option is called inState#set
(as equals-ish doesn't dirty it), but before everything inComputed#get
.token
is whatever your favorite cancellation object is. If it cancels the last promise before it resolves, the same pre-resolve job above is executed immediately without scheduling, and the resolvers are dropped.signal.get()
, if the promise resolver list is empty and the "track" flag is set, the flag should be cleared then theuntrack
hook called synchronously. This needs done after everything else, to allow cells to re-watch inside their bodies (and avoid needless untrack/track calls).signal.isTracked
returns the value of that "track" flag.Signal.subtle.stopTrackingUpdates(signal)
drops all promises, then does the same final cleanup thatsignal.get()
does.Here's my line of thinking:
async
/await
much better.if (!signal.isDirty) await Signal.subtle.waitForUpdate(signal); schedule(doGet)
where you entirely avoid waiting at all.The text was updated successfully, but these errors were encountered: