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

fix(Notification): ensure do and observe handle partial observation #1260

Closed
wants to merge 1 commit into from
Closed

fix(Notification): ensure do and observe handle partial observation #1260

wants to merge 1 commit into from

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Jan 28, 2016

  • do will now handle notifying a handler that isn't present. (e.g. notification.do(null, errorFn, null))
  • observe will now handle partial observers.

Relates to #1211...

Rather than enforce the shape of the Observer passed to observe, we can allow any shape of Observer. If someone wants to use a generator as an observer passed to Notification.prototype.observe I see no reason to prevent them just because they don't have error or complete functions.

- `do` will now handle notifying a handler that isn't present. (e.g. `notification.do(null, errorFn, null)`)
- `observe` will now handle partial observers.
@@ -11,11 +11,11 @@ export class Notification<T> {
observe(observer: Observer<T>): any {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type definition still requires observer interface, so typescript won't be able to pass partial implementation of observers. Should it be updated in this PR, or leave it for further type enhancement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I thought I also had a PR to change the Observer interface to allow partials as well...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, https://github.com/ReactiveX/RxJS/blob/master/src/Observer.ts#L3-L5 seems we may need those changes also. (better to include in this PR as one shot changes?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just filed an issue related to this here: microsoft/TypeScript#6806

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This microsoft/TypeScript#4889 might be relevant to what we want?

@benlesh
Copy link
Member Author

benlesh commented Feb 2, 2016

Closing in favor of #1282

@benlesh benlesh closed this Feb 2, 2016
@benlesh benlesh deleted the notification-protect branch April 27, 2016 17:20
@lock
Copy link

lock bot commented Jun 7, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants