-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add ES7 start event handler to Observer, Observable and Subject #726
Conversation
} | ||
|
||
subscriber.start(subscriber); | ||
subscriber.add(this._subscribe(subscriber)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Blesh should this line be protected with a conditional? if (!subscriber.isUnsubscribed) { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subscribers should always have a start method. The conditional is in the Subscriber.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Blesh yes, but if the start
method synchronously calls unsubscribe
(thus setting isUnsubscribed
to true
), then there's no reason to execute line 104 (subscriber.add(this._subscribe(subscriber));
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
checks isUnsubscribed
internally. I could do that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Blesh yes, but that happens after _subscribe
runs. Don't you think it'd be more efficient if we didn't even execute the subscription if all we do is immediately throw it all away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I thought part of the purpose was to actually call the _subscribe
and let user logic decide what to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Blesh ah, I wasn't aware of that requirement. I'm thinking if subscribing is referentially transparent, and subscribing performs some action, then unsubscribing performs the inverse, then synchronous subscribe + unsubscribe is equivalent to not even subscribing in the first place. Does that sound right? What could the subscription do with a subscriber that's already disposed by the time it's called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In es-observable, I'm not calling the subscribe function if the subscription is already closed. I think it would be surprising for users to receive a closed subscription.
new Observable(subscriber => {
// I'll probably write this assuming that subscriber represents an active subscription
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh? For some reason I thought that was part of this feature. "Look it's unsubscribed, I should do something different (like error or notify someone)"
I'm fine with this change though, it's minimal.
I reviewed all commits. Looks good and useful 👍 |
Change looks OK for me. |
I'm going to leave this one for a few more days while I wait to hear back from @jhusain and @zenparsing on what's happened with the spec at the TC39 this week. |
Adding a handler for start that gives you the subscription at the moment subscription starts this is to facilitate synchronous unsubscribe, and to match the ES7 observable spec related tc39/proposal-observable#65 related #716
In order to support synchronous unsubscription, a fourth handler was added that will trigger at the moment of subscription. This is to support the ES7 observable spec as well. - add start handler to Observer interface - add call to start handler on subscription - add start handler to Subscriber - add start handler parameter to Subscriber.create related tc39/proposal-observable#65 related #716
- incoming start requests are ignored for base Subject, which does not care about them - when an observer subscribes, its start event is notified prior to it being added to the observers list in the event of a synchronous unsubscribe, it will simply not be added to the observers list related #716
- stubs in start event for basic Subject which does not care about them - calls start on subscribing Observers prior to adding them to the observers list closes #716
Closing this one for now, as it doesn't look like it's going to get passed the committee. |
Sad to have coded it in vain. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
See commit messages for more detailed explanations, but the high level is this:
start
is a handler onObserver
that is called synchronously on subscription before the Observable's initializer is called. This enables "synchronous unsubscribe". This is all part of the ES7 Observable spec (issue around it can be found here: tc39/proposal-observable#65)For Subjects, after some discussion with @abersnaze of RxJava noteriety, it was determined that the basic
Subject
should noop incoming start messages it observes. However, whensubscribe
is called on the Subject, the start handler on the passed observer will be called prior to adding that observer to the Subject'sobservers
list.Since this is a critical change, with interesting discoveries...
cc/ @trxcllnt @staltz @trxcllnt @mattpodwysocki @zenparsing @jhusain @kwonoj