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

Adding/removing watchers during execution of set #210

Closed
prophile opened this issue Apr 29, 2024 · 2 comments
Closed

Adding/removing watchers during execution of set #210

prophile opened this issue Apr 29, 2024 · 2 comments

Comments

@prophile
Copy link
Contributor

prophile commented Apr 29, 2024

In "Method: Signal.State.prototype.set(newValue)", we have:

6: For each previously ~watching~ Watcher encountered in that recursive search, then in depth-first order,
i. Set notifying to true while calling their notify callback (saving aside any exception thrown, but ignoring the return value of notify), and then restore notifying to false.

A watcher's notify callback is permitted to call .watch or .unwatch (neither of their algorithms specify a notifying === false check) and it's not clear what's supposed to happen in that case – if one watcher calls .unwatch on another which would be notified by this algorithm but has not yet been so notified, should this stage of the algorithm still notify it or not?

Consider the following code:

const state = new Signal.State(0)
const watcherA = new Signal.subtle.Watcher(() => {
  console.log("notified A")
});
const watcherB = new Signal.subtle.Watcher(() => {
  console.log("notified B")
  watcherA.unwatch(state);
});
watcherB.watch(state);
watcherA.watch(state);
state.set(1);

Should this print "notified B" and "notified A", or just "notified B"?

My reading of the current specification is that "previously ~watching~ Watcher" implies it should log both because they were both encountered in the search before running callbacks, but the polyfill in f7c550b just logs "notified B".

@robbiespeed
Copy link

If I'm not mistaken adding watchers during notify already is an error, so it would follow that removing them should be as well.

This is definitely an area that needs further definition either way though, especially considering watcher.watch() currently acts as a reset (something that needs to be defined further as well) and will never error in notify, but watcher.watch(signal) will error.

@littledan
Copy link
Member

Fixed by #223

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

No branches or pull requests

3 participants