-
Notifications
You must be signed in to change notification settings - Fork 14
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
Is there any concern about the difference in constructor semantics from Promise
?
#22
Comments
That cat is sort of already out the the bag for more than a decade now. It's a non-issue in terms of broad understanding of the type. I know that's dismissive, so I'll try to elaborate. However, to play devil's advocate, there are other APIs that don't quite line up with what the const resizeObserver = new ResizeObserver((entries) => {
// do stuff
});
resizeObserver.observe(element);
resizeObserver.disconnect();
const mutationObserver = new MutationObserver((entries) => {
// do stuff
});
mutationObserver .observe(element);
resizeObserver.disconnect(); And honestly, those two APIs would have been so much better off as observables, because as they're currently implemented, while you can In terms of unicast/multicast, which is this issue: const expensiveThingObservable = new Observable((subscriber) => {
subscriber.next(someReallyExpensiveComputation());
}); This is already something people face with ordinary functions: const getExpensiveThing = (callback) => {
callback(someReallyExpensiveComputation())
} And the solution is exactly the same: let firstTry = true
let expensiveThing = null
const getExpensiveThing = (callback) => {
if (firstTry) {
firstTry = false;
expensiveThing = someReallyExpensiveComputation();
}
return expensiveThing;
} Again, identical with an observable: let firstTry = true
let expensiveThing = null
const expensiveThingObservable = new Observable((subscriber) => {
if (firstTry) {
firstTry = false;
expensiveThing = someReallyExpensiveComputation();
}
subscriber.next(expensiveThing);
subscriber.complete();
}); Now when it's asynchronous, it's just a little different: let waiting = true
let expensiveThing = null
let deferred = new Set()
const expensiveThingObservable = new Observable((subscriber) => {
if (waiting) {
someReallyExpensiveAsyncThing((err, value) => {
waiting = false;
expensiveThing = value;
const subscribers = Array.from(deferred);
for (const s of subscribers) {
if (err) {
s.error(err);
} else {
s.next(value);
s.complete();
}
}
})
} else {
subscriber.next(expensiveThing);
subscriber.complete();
}
}); BUT: No one should be using an observable for an asynchronous single value, that's silly. Just use a promise for that. But even that comes with gotchas. For one thing, it's not an amazing idea to keep a promise around as a means of caching a value. Some promises can close over scope that is retained for the life of the promise, that's a runtime implementation thing, but it's still worth avoiding. So even with promises, the "expensive calculation" issue gets a little weird: let promise;
let waiting = true;
let expensiveThing;
async function getExpensiveThingAsync() {
if (waiting) {
promise ??= new Promise((resolve, reject) => {
someReallyExpensiveAsyncThing((err, value) => {
waiting = false;
if (err) {
reject(err);
return;
}
expensiveThing = value;
resolve(value);
})
});
return promise;
}
return expensiveValue;
} In the end, |
But are there API decisions that could alleviate this concern? Like an Observable constructor could look more like ReadableStream: const subscribers = new Map();
const observable = new Observable({
subscribe(subscriber) {
subscriber.next(1);
...
subscribers.add(subscriber, metadata);
},
abort(subscriber) {
...
subscribers.delete(metadata);
},
}); In this example, the This approach doesn't change behavior at all, it just merely changes the shape of the constructor so that it's less of a Promise-look-alike. |
That would really complicate the type. You can do what you're proposing above pretty easily with the proposed API: const subscribers = new Map();
const observable = new Observable((subscriber) => {
subscriber.next(1);
...
subscribers.add(subscriber, metadata);
return () => {
subscribers.delete(metadata);
};
});
I'm not sure how to phrase this without it reading as snarky, so apologies in advance: But |
Fair enough, thanks for your responses and thoughts. Probably worth leaving open in case someone else has the same thought, but as far as resolution it's your speculation that people won't get confused vs mine that that they will, and your speculation is based on a lot more experience with people using But it could be worth finding a more rigorous way to answer this question by reaching out to a broader set of people to hear their opinions as well. Now is the time to figure out if API shape deviations from RxJS |
Tom asked me to chime in, so my thoughts:
|
That's fair, it's not for everyone.
It is, but I'm concerned about introducing anything that isn't battle tested. One thing that the original proposal for Observable for the TC39 lacked was a way to set up teardown during the "start" of the subscription. RxJS solved this by having an That said, I'm open to an API like this if it works and makes sense, as it could easily be added to existing implementations like RxJS in a non-breaking way. Here's a use case, just as an example, perhaps someone sets up an Observable that starts two different resources, that could error during start up: Traditional (TC39) ObservableThere's a lot for the user to do to make sure this is safe, which is mostly okay, because it's a pretty basic type. Yow might have to do similar things to reject in a To complicate things, the returned teardown function is actually added to the subscription after that subscription would already be unsubscribed (implicitly by the new Observable(subscriber => {
let resource1;
let resource2;
try {
resource1 = new Resource1(value => {
subscriber.next({ from: 1, value });
});
resource2 = new Resource2(value => {
subscriber.next({ from: 1, value });
});
} catch (error) {
// make sure to communicate the error to the consumer.
subscriber.error(error);
} finally {
// make sure we attempt to teardown
return () => {
resource1?.disconnect();
resource2?.disconnect();
}
}
}) RxJS ObservableRxJS attempts to handle all of this by automatically forwarding any errors thrown in the "start" to the This is a little more straightforward, and could easily be rewritten to use new Observable(subscriber => {
let resource1;
let resource2;
subscriber.add(() => {
resource1?.disconnect();
resource2?.disconnect();
});
resource1 = new Resource1(value => {
subscriber.next({ from: 1, value });
});
resource2 = new Resource2(value => {
subscriber.next({ from: 1, value });
});
}) Hypothetical Config-based Observable CtorThis has a few issues. For one thing, how do we create a new resource per-subscription, to share between the two callbacks? We'd need to have some sort of state setup function or the like? new Observable({
start(subscriber) {
// Todo, create two resources.
},
cancel() {
// Todo, teardown those resources if necessary
}
}) We could do something to have a callback that initializes some shared state on subscription, but it seems needlessly complicated. new Observable({
initState() {
return { resource1: null, resource2: null }
},
start(subscriber, state) {
state.resource1 = new Resource1(value => {
subscriber.next(value);
});
state.resource2 = new Resource2(value => {
subscriber.next(value);
});
},
cancel(state) {
state.resource1?.disconnect();
state.resource2?.disconnect();
}
}) Alternatively, we could have something that returns the configuration from within a closure to allow the resources to be created, but I have zero doubt that people will mess this up as well new Observable(() => {
let resource1;
let resource2;
return {
start(subscriber) {
resource1 = new Resource1(value => {
subscriber.next(value);
});
resource2 = new Resource2(value => {
subscriber.next(value);
});
},
cancel() {
resource1?.disconnect();
resource2?.disconnect();
}
}
}) Anything else I'm thinking of here seems like it would force eagerness and multicast (closing over outside instances of the resources, manual reference counting, etc), which really dilutes the possible uses of the type and really makes it not an observable. The crux of it is that the |
(I'm truly sorry that all of this ends up being a novel, I'm just trying to be thorough 😅 ) |
The way we'd handle that with the streams-like pattern is as follows: new Observable({
start(subscriber, state) {
this.resource1 = new Resource1(value => {
subscriber.next(value);
});
this.resource2 = new Resource2(value => {
subscriber.next(value);
});
},
cancel(state) {
this.resource1?.disconnect();
this.resource2?.disconnect();
}
}); |
I'm amenable to this design. My only concern would be it's new. I'd want to fiddle with it to see if it was able to meet the same needs/use cases. One concern is that it would force it to be multicast, or require ref counting. But I suppose it could be made to not work like that. But maybe that's not a big deal. It requires thought. |
I guess the |
So we're discussing the possibility of having a bag of options passed into the Observable constructor, instead of just a callback which is responsible for managing teardown from within itself. This means we'd have to have a way to share state between the passed-in start/subscribe callback, and the stop/cancel/teardown callback. But I'm a little confused about what was posed in #22 (comment). There are three pieces of state there:
|
The latter was the intent. But if I understand correctly, this is bad: you need per-subscription state, and there's only a single per- If I understand correctly that you need per-subscription state, then I rescind my idea: it is not a good fit and will be a footgun. Given that I'm not sure where we are with a solution to this issue. My idea, with |
I definitely share your opinion about not liking the "return a cleanup function" from the subscriber callback (i.e., the callback that gets passed into the Observable ctor, just so we're all clear). It seems very un-idiomatic. Since the |
Observables have a strict requirement that `complete()` and `error()` can only ever be called once, and specifically, once either one of these runs, no other `Observer` method can ever run again. The experimental Observable implementation in Blink previously did not respect this constraint, and this CL fixes this by essentially "closing" the subscription. ===Subsequent work=== Properly "closing" a subscription involves: 1. Ensuring `Subscriber#isActive` becomes false (WICG/observable#76 will track this). 2. Aborting the Subscriber's `AbortSignal`. This requires the AbortSignal that lives on `Subscriber` to differ from the one passed into `subscribe()`. See crrev.com/c/5017401. 3. Running the teardowns as the AbortSignal algorithms. Future CLs will implement the `addTeardown()` method in accordance with discussion in WICG/observable#22 (comment). R=masonf@chromium.org Bug: 1485981 Change-Id: I4ef9ab438e59326a49bd6561108b3bcc736845a5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5017293 Auto-Submit: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1223921}
See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca
See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca
See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca
See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1227535}
See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1227535}
See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1227535}
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: ben@benlesh.com R=masonf@chromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265
Currently the explainer & Chromium implementation (behind a flag) has been using the |
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: benbenlesh.com R=masonfchromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <domchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265 UltraBlame original commit: 063701177178e391eacb3c525fb8a5339b177d0b
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: benbenlesh.com R=masonfchromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <domchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265 UltraBlame original commit: 5d1528c1841d314e74a855ee483fcca04ddf1cd8
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: benbenlesh.com R=masonfchromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <domchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265 UltraBlame original commit: 063701177178e391eacb3c525fb8a5339b177d0b
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: benbenlesh.com R=masonfchromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <domchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265 UltraBlame original commit: 5d1528c1841d314e74a855ee483fcca04ddf1cd8
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: benbenlesh.com R=masonfchromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <domchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265 UltraBlame original commit: 063701177178e391eacb3c525fb8a5339b177d0b
…for Observables, a=testonly Automatic update from web-platform-tests DOM: Introduce Subscriber#addTeardown() for Observables See https://github.com/WICG/observable/blob/master/README.md and the design discussion in WICG/observable#22. For WPTs: Co-authored-by: benbenlesh.com R=masonfchromium.org Bug: 1485981 Change-Id: I1de6ff5813f1c9ea6762fb4569440350c5dc38ca Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018147 Commit-Queue: Dominic Farolino <domchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1227535} -- wpt-commits: 6bbd0ddcb078ca56533085359b4b5fa5ed36ee2f wpt-pr: 43265 UltraBlame original commit: 5d1528c1841d314e74a855ee483fcca04ddf1cd8
To preface, I understand that RxJS and other Observable implementations are widely used and very popular, and that a lot of people have been very successfully using Observables for very long time in many applications.
But there's a difference between using a library and using a web platform primitive. And I do worry a little that people will expect the
Observable
constructor to behave similarly to thePromise
constructor, in particular being executed synchronously and only once for every.then()
callback. Observables don't really work that way, as the callback parameter is called whensubscribe
is called, an unlimited number of times. This is the much discussed cold vs hot distinction.So for example, maybe it's really easy to write:
And people comfortable with promises will think that's only going to be executed once and then each subscriber gets the expensive value, when in reality the expensive computation happens once for every subscriber.
This is less of a concern for Observables that the platform creates for something like event listeners, where the browser is in charge of the observable instantiation and the rest of the API is just really offering a better listener pattern.
The text was updated successfully, but these errors were encountered: