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

Add subscription as secondary argument to next handlers for Observers #3983

Closed
benlesh opened this issue Aug 2, 2018 · 8 comments
Closed

Comments

@benlesh
Copy link
Member

benlesh commented Aug 2, 2018

Currently, you can get a hold of the subscription in an Observer or next handler via the this context:

range(0, Number.POSITIVE_INFINITY)
  .subscribe(function (value) {
    console.log(value);
    if (value >= 3) this.unsubscribe();
  });

// logs
// 0
// 1
// 2
// 3

Most people aren't aware of this feature or why it exists. It exists for the reason you see above. If you have a synchronous fire hose of values, you need some way to shut it off, and you don't have a handle to the subscription beforehand. If you use a non-arrow function here, you can use this to get at the unsubscribe and tear down if necessary.

It's cool, but not intuitive

Proposal

Add subscribe as the second argument to next handlers passed to subscribe.

  • It works in all cases below:
source.subscribe((value: T, subs: Subscription) => console.log(value));
source.subscribe(function (value: T, subs: Subscription) => console.log(value));
source.subscribe({
  next(value: T, subs: Subscription) { console.log(value); );
source.subscribe({
  next: (value: T, subs: Subscription) => console.log(value)
});
source.subscribe({
  next: function (value: T, subs: Subscription) { console.log(value); }
});
  • It's much more discoverable by TypeScript users via IDEs and the like
  • It aligns a bit more with the WHATWG proposal, FWIW
@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Aug 2, 2018
@jayphelps
Copy link
Member

jayphelps commented Aug 2, 2018

Any thoughts on how operators that accept an Observer like tap would be affected?

It seems like a footgun to provide the subscription to Observers provided to tap and not providing it would break type systems.

rough TypeScript Playground example

You're probably proposing that Observer's interface stays the same, but that subscribe accepts a type of observer that is a superset of Observer where the next() function has the subscription? So if someone provides a regular, true Observer, they won't see the second argument to next() because Observer's type signature doesn't have it. I think that works, even if it might be a bit confusing. This would also play nicely with the statement that Subjects are Observers (cause you wouldn't be providing a sub to subject.next(value))

@benlesh
Copy link
Member Author

benlesh commented Aug 2, 2018

Any thoughts on how operators that accept an Observer like tap would be affected?

tap actually has the name access as subscribe does now...

range(0, Number.POSITIVE_INFINITY).pipe(
  tap(function (value) {
    console.log(value);
    if (value >= 3) this.unsubscribe();
  })
).subscribe();

Given that, I think it's probably best to keep it congruent with subscribe.

Subjects tho

Subjects are another thing to think about. It doesn't make sense to require that Subject.next takes a subscription argument. So there I think we won't do that. Because if we did, then what? Pass it through? All subscribers to subjects already have their own Subscription instances.

@jayphelps
Copy link
Member

Given that, I think it's probably best to keep it congruent with subscribe.

Is that intentional or just incidental from sharing code? I'm personally against it, but it's not a huge deal, just a footgun IMO.

@benlesh
Copy link
Member Author

benlesh commented Aug 2, 2018

Is that intentional or just incidental from sharing code

Intentional. tap can be used to effectively wrap the subscription to an observable (and all that you can do there) in another observable. It's just one of many uses of tap.

@benlesh
Copy link
Member Author

benlesh commented Aug 15, 2018

Note from core team meeting: We might want it to be a SubscriptionLike rather than a full Subscription.

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Aug 15, 2018
@Airblader
Copy link
Contributor

It exists for the reason you see above.

Does it really, as in this has been explicitly designed this way? I would've assumed this was just an unintended coincidence.

It's cool, but not intuitive

That's not the kind of argument a non-core team member could ever make to get something approved :-)

My question is: what are such fire hoses useful for? Seems like this is just a "reactive" way of creating a specific array, which to me sounds like something that could be handled entirely outside rxjs, and then be passed to from?

@gustavshf
Copy link

It exists for the reason you see above.

Does it really, as in this has been explicitly designed this way? I would've assumed this was just an unintended coincidence.

It's cool, but not intuitive

That's not the kind of argument a non-core team member could ever make to get something approved :-)

My question is: what are such fire hoses useful for? Seems like this is just a "reactive" way of creating a specific array, which to me sounds like something that could be handled entirely outside rxjs, and then be passed to from?

Such hoses are useful for unit tests.

@benlesh
Copy link
Member Author

benlesh commented Sep 2, 2020

I think we can close this, general lack of interest.

@benlesh benlesh closed this as completed Sep 2, 2020
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

4 participants