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

Hard bound methods when creating new observer #3842

Closed
SanderElias opened this issue Jun 16, 2018 · 7 comments
Closed

Hard bound methods when creating new observer #3842

SanderElias opened this issue Jun 16, 2018 · 7 comments

Comments

@SanderElias
Copy link

SanderElias commented Jun 16, 2018

Bug Report/ Feature request

Current Behavior
The follow code fails because the methods on the subscriber are not hard bound to it.

Reproduction

const mySpecialTimer = new Observable(({ next, complete }) => {
  let count = 1;
  const rinse = () => setTimeout(() => {
    next(++count);
    if (count < 15) {
      return rinse();
    }
    complete();
  }, 500);
  rinse();
  return () => console.log('done');
}).pipe(take(5)).subscribe(console.log);

Expected behavior
That the code works as expected. I'm not sure if this is a bug or a feature request thought.

Possible Solution
Hard bound methods on observable

@cartant
Copy link
Collaborator

cartant commented Jun 16, 2018

I would not call this a bug. The documentation is pretty clear that what's passed is a Subscriber, so I'm not sure why you'd expect to be able to extract methods from it anymore than, say, you'd expect to be able to extract subscribe from an Observable instance. Should all methods be hard-bound to an instance?

A somewhat related issue: #3229

@SanderElias
Copy link
Author

@cartant If possible, yes, as it leads to less unexpected behaviours. I came to this after I ran into trouble doing something like this when creating an observable

    return new Observable<any>(obs => {
      this.sock.on(eventName, obs.next);
      this.sock.on('disconnect',obs.complete)
      return () => {
        console.log('socket cleaned');
      };
    });

I know the workaround, but as I like functional programming, and would prefer things like this to work.
Also, doing it like this is similar on how promises are created.

@cartant
Copy link
Collaborator

cartant commented Jun 16, 2018

Yeah, but with promises, the resolve and reject functions are passed as separate parameters.

I'm not sure I agree with the effected behaviour being "unexpected", as the methods have to be explicitly extracted using destructuring - or whatever.

Also, binding the methods to the instance would incur a performance penalty: next, error and complete (and perhaps more, as the API explicitly states that it's a Subscriber and not an Observer that's passed) would all need to be bound - whether used or not. And for no reason other than a stylistic convenience.

Maybe there are some implementation changes in the experimental branch that might be - or could be - more inline with what you are looking for, but I'm only just starting to familiarise myself with the work that's happening there. However, my understanding is that subscribers are still going to be "a thing", so there will still be parts of the API that deal with objects, rather than just functions.

@kwonoj
Copy link
Member

kwonoj commented Jun 16, 2018

I agree with @cartant here in general, this is neither bug nor unexpected behavior. Subscriber never explicitly said to be context binded and it was not a design goal. It is still possible eventually we could have it, but we don't actively pursue as goal compare to other design decisions core trying to achieve at this moment.

@SanderElias
Copy link
Author

About unexpected, #3229 is an example. But I'm not going to pressure here.
As I said, not sure if this was a bug or a feature request.

All I'm saying is that the developer ergonomics of the subscriber usage would improve when this is implemented. It will enable a more functional way of programming. With the JS destructuring this will enable a more clean way to create observables.
another example:

const documentClicks$ = new Observable(({ next }) =>
  document.addEventListener('click', next)
);

I think that is a nice an concise style of programming. (I know about fromEvent, this is a sample on style, not on functionality)

@cartant
Copy link
Collaborator

cartant commented Jun 16, 2018

@SanderElias Sure, but at some point it will be necessary to accept that the API involves subscriptions and subscribers and that they are object instances. After all, this will fail:

const source = of(1);
const { unsubscribe } = source.subscribe();
unsubscribe();

Also, new Observable is also a low-level API - as you've noted, users are encouraged to create observables using factory functions, like fromEvent, wherever possible - and binding the methods would add overhead. I've more than a few operator implementations that use new Observable and simply pass the subscriber to an inner obervable's subscribe.


Maybe if a factory-function equivalent of new Observable is added - I have vague memories of such a thing being mentioned somewhere - it could take hard-bound functions, but I think that new Observable should remain as-is and without any binding overhead - however small that may be.

@SanderElias
Copy link
Author

hmm, A factory function would do it alright. I understand that the (although small) overhead adds up, and will be a chore on creating operators.

It would be cool if such a factory would be part of rxjs. If not, I will create one for myself.
Thanks for the explanation.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 16, 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

No branches or pull requests

3 participants