Skip to content

Move core operators onto Observable directly #468

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

Closed
benlesh opened this issue Oct 6, 2015 · 10 comments
Closed

Move core operators onto Observable directly #468

benlesh opened this issue Oct 6, 2015 · 10 comments

Comments

@benlesh
Copy link
Member

benlesh commented Oct 6, 2015

As an aside to the discussion on #464 ... We might want to move all core operators directly onto the Observable itself.

I'm on the fence about this one.

  1. It makes overriding operators for better performance on Observables like ScalarObservable and ArrayObservable more straightforward, and require less type checking in the operator implementations themselves.
  2. It's less hacky. We'll no longer be mutating the prototype in a separate module from Observable for our main operators
  3. It's unlikely most people will want to use an Observable with no operators.

... but I'll really want to trim down what is "core". For example, share() and multicast() should be core, since share is very commonly used, and multicast enables all other variants... but shareReplay, shareBehavior, publish, publishReplay, publishBehavior should probably not be "built in", IMO.

@benlesh
Copy link
Member Author

benlesh commented Oct 6, 2015

attn @jeffbcross

@staltz
Copy link
Member

staltz commented Oct 7, 2015

There is no point keeping share and leaving out shareReplay, shareBehavior, they are all equal specializations of multicast. Better just keep multicast.

  • If it can be done trivially with an existing operator or two, it's not core.
  • if it cannot be done trivially with an existing operator, it should be considered for core.

share === multicast(() => new Rx.Subject()) looks trivial, so share is not core (?).

@benlesh
Copy link
Member Author

benlesh commented Oct 7, 2015

they are all equal specializations

I guess I'm unconvinced that they're as commonly used, even if they are very similar.

@staltz
Copy link
Member

staltz commented Oct 7, 2015

In Cycle.js world, definitely. In other contexts, I don't really know how RxJS is used in Netflix, neither in Angular2.

@jeffbcross
Copy link
Contributor

I don't really know how RxJS is used in Netflix, neither in Angular2.

Neither do I! Actually, we right now only publish observables for component events, and for http requests. We don't consume them internally, except for in our examples. But the lighter the "core" set of operators can be, the better, as discussed in this issue.

@benlesh
Copy link
Member Author

benlesh commented Oct 8, 2015

cc @steveorsomethin @jhusain

@robwormald
Copy link
Contributor

In Typescript and with generated type definitions, the way the prototype is currently mutated doesn't complain internally, but also doesn't offer autocomplete or anything useful, which is sort of a bummer when y'all are making all this effort with Typescript and tooling.

screen shot 2015-10-11 at 10 56 44 pm

@benlesh
Copy link
Member Author

benlesh commented Oct 12, 2015

Yeah, we'll probably have to go back to at least adding stubs for every operator on the Observable type. It's just so ugly.

Part of me wonders if it might be better just to switch to ES6 and manually maintain the .d.ts files...

@david-driscoll
Copy link
Member

I've done some work to that effect, that makes consuming the typings easier, and all the stubs only have to go into the operators themselves.

#715

@benlesh
Copy link
Member Author

benlesh commented Nov 30, 2015

I think we've decided not to do this.

@benlesh benlesh closed this as completed Nov 30, 2015
@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

No branches or pull requests

5 participants