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

Determine which Observable operators and extensions to include in angular core #4390

Closed
robwormald opened this Issue Sep 28, 2015 · 30 comments

Comments

Projects
None yet
8 participants
@robwormald
Member

robwormald commented Sep 28, 2015

Now that #2974 has landed, we're exposing the "bare" RxNext Observable without any operators included. This is partially because including the main Rx import caused everything to explode (as it has a large amount of files) and partially to avoid bloating the core codebase with another ~125k of operators.

The list of currently implemented operators is here: https://github.com/ReactiveX/RxJS/tree/master/src/operators

A core set of of them should be included, things like map, flatMap, flatMapLatest, take, filter, reduce, scan, merge, and toPromise. Additionally specific operators can be exposed for specific APIs - for example, Http should add retry and retryWhen (or some equivalent)

Also, a set of specific Observable utility implementations are available - things like Observable.from, Observable.fromArray, Observable.interval, etc. The list of these is here: https://github.com/ReactiveX/RxJS/tree/master/src/observables

Alternately we could fix the build issue and simply bring in the entire Rx module for now, and pare it down as we move closer to production and begin to optimize.

cc @jeffbcross @vsavkin

edit: the ES6 version is ~250k for the complete unminiified Rx, roughly ~half of which is operators. For CJS, its ~350k with again roughly half being operators.

@gdi2290

This comment has been minimized.

Show comment
Hide comment
@gdi2290

gdi2290 Sep 28, 2015

Member

can you also include Observable.of and Observable.defer?

Member

gdi2290 commented Sep 28, 2015

can you also include Observable.of and Observable.defer?

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Sep 28, 2015

Contributor

It would be nice to provide a minimum set of operators, as you've described, by default from Angular APIs that export Observables. And to allow users who'd like a different set of operators, they should be able to override an ObservableFactory binding with DI (name not important right now). So when overriding that binding, calling http.get() would return an observable with the operators I need.

Contributor

jeffbcross commented Sep 28, 2015

It would be nice to provide a minimum set of operators, as you've described, by default from Angular APIs that export Observables. And to allow users who'd like a different set of operators, they should be able to override an ObservableFactory binding with DI (name not important right now). So when overriding that binding, calling http.get() would return an observable with the operators I need.

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Sep 28, 2015

Contributor

If we include an Observable factory in DI, it's worth considering whether the same should be done for Promise.

Contributor

jeffbcross commented Sep 28, 2015

If we include an Observable factory in DI, it's worth considering whether the same should be done for Promise.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Sep 28, 2015

Member

👍 and 👍 (1 for Observable and 1 for Promise factory).

Member

gkalpak commented Sep 28, 2015

👍 and 👍 (1 for Observable and 1 for Promise factory).

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Sep 29, 2015

Member

I would vote to not include any in Angular2 and have people import what they need.

Member

mhevery commented Sep 29, 2015

I would vote to not include any in Angular2 and have people import what they need.

@robwormald

This comment has been minimized.

Show comment
Hide comment
@robwormald

robwormald Sep 29, 2015

Member

Problem with that is at the moment there's not really a good way to add them after the fact to Observables returned from the core, at least in TS:

//without operators:
http.get('foos.json').subscribe(res => {
  this.foos = res.json();
});

//with .map included, connected to an async pipe
this.foos = http.get('foos.json').map(res => res.json());

//with .retry() included
this.foos = http.get('foos.json').retry(3).map(res => res.json());

The ES7 functionBind proposal gives us a nice solution, but its not currently supported in TS:

import {map, retry} from './myoperators'

this.foos = http.get('foos.json')::retry(3)::map(res => res.json());

Dart has a distinct advantage here as they provide a similar set in the language itself on Streams:
https://api.dartlang.org/1.12.1/dart-async/Stream-class.html#instance-methods

Member

robwormald commented Sep 29, 2015

Problem with that is at the moment there's not really a good way to add them after the fact to Observables returned from the core, at least in TS:

//without operators:
http.get('foos.json').subscribe(res => {
  this.foos = res.json();
});

//with .map included, connected to an async pipe
this.foos = http.get('foos.json').map(res => res.json());

//with .retry() included
this.foos = http.get('foos.json').retry(3).map(res => res.json());

The ES7 functionBind proposal gives us a nice solution, but its not currently supported in TS:

import {map, retry} from './myoperators'

this.foos = http.get('foos.json')::retry(3)::map(res => res.json());

Dart has a distinct advantage here as they provide a similar set in the language itself on Streams:
https://api.dartlang.org/1.12.1/dart-async/Stream-class.html#instance-methods

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Sep 30, 2015

Contributor

Without es7 function bind support in TypeScript, I'd recommend just including the current RxJS 5 Core Operators. There are a lot, but frankly they're a subset, and they're things that people want.

If you really don't want to include those, you could just mirror the Rx.ts file and create your own "lite" version... But then you'll be stuck supporting requests for the other operators.

... So my recommendation is use Rx as-is. And come make arguments to have ones removed you don't think are "core" enough.

Contributor

benlesh commented Sep 30, 2015

Without es7 function bind support in TypeScript, I'd recommend just including the current RxJS 5 Core Operators. There are a lot, but frankly they're a subset, and they're things that people want.

If you really don't want to include those, you could just mirror the Rx.ts file and create your own "lite" version... But then you'll be stuck supporting requests for the other operators.

... So my recommendation is use Rx as-is. And come make arguments to have ones removed you don't think are "core" enough.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Sep 30, 2015

Contributor

FWIW, the ones in RxJS 5 Core are ones that are non-trivial to reproduce with any combination of other operators. That's the goal at least.

Most of them were chosen because of their usage within Netflix apps.

Contributor

benlesh commented Sep 30, 2015

FWIW, the ones in RxJS 5 Core are ones that are non-trivial to reproduce with any combination of other operators. That's the goal at least.

Most of them were chosen because of their usage within Netflix apps.

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 5, 2015

Contributor

We had a meeting with @IgorMinar, @blesh and @robwormald today to discuss this issue. We wanted to come to a solution that would allow us to keep the byte size of angular2 core small, and make it easy for Rx power-users to easily get useful operators.

My proposal was to use a "lite" Observable with only a subscribe method, and use DI and provide an ObservableFactory token that could be overwritten:

import {Observable} from '@reactivex/rxjs/Rx';
// Bind the included factory to a full-featured Observable with default set of operators
bootstrap(MyComponent, [bind(ObservableFactory).toValue(Observable)]);

@mhevery brings up the point that by requiring an injector, users could not take advantage of field assignment shorthand provided by TypeScript.

class MyComponent {
  someEvent = new EventEmitter(); // EventEmitter couldn't use DI in this case, unless refactored to accept an Observable argument.
}

My counter-argument to that is that if users wanted to instantiate their own Subjects/Observables, they could just import the one the same one they've already bound to ObservableFactory token, and just instantiate it themselves without DI. It's up to whomever is consuming the component's events to know the shape of the Observable being provided. The benefit of the DI binding should only be for the angular framework itself to produce the shape of Observable that the consumer would like.

However, there are still issues with type information with the DI approach. For angular APIs that say they return an Observable from Angular (Observable lite), any code that consumes such an Observable would not automatically know that the binding had been replaced with a new shape of Observable.

Another discussion topic that came up was strategies for composing operators. Right now it's possible to compose operators onto an Observable instance using the lift method. The current RxJS design is prototype-based, so monkey-patching or extending are the way to add default operators.

Random notes, I know, but wanted to get them out there before I forget.

Did I miss or misrepresent anything @blesh @robwormald @IgorMinar?

Contributor

jeffbcross commented Oct 5, 2015

We had a meeting with @IgorMinar, @blesh and @robwormald today to discuss this issue. We wanted to come to a solution that would allow us to keep the byte size of angular2 core small, and make it easy for Rx power-users to easily get useful operators.

My proposal was to use a "lite" Observable with only a subscribe method, and use DI and provide an ObservableFactory token that could be overwritten:

import {Observable} from '@reactivex/rxjs/Rx';
// Bind the included factory to a full-featured Observable with default set of operators
bootstrap(MyComponent, [bind(ObservableFactory).toValue(Observable)]);

@mhevery brings up the point that by requiring an injector, users could not take advantage of field assignment shorthand provided by TypeScript.

class MyComponent {
  someEvent = new EventEmitter(); // EventEmitter couldn't use DI in this case, unless refactored to accept an Observable argument.
}

My counter-argument to that is that if users wanted to instantiate their own Subjects/Observables, they could just import the one the same one they've already bound to ObservableFactory token, and just instantiate it themselves without DI. It's up to whomever is consuming the component's events to know the shape of the Observable being provided. The benefit of the DI binding should only be for the angular framework itself to produce the shape of Observable that the consumer would like.

However, there are still issues with type information with the DI approach. For angular APIs that say they return an Observable from Angular (Observable lite), any code that consumes such an Observable would not automatically know that the binding had been replaced with a new shape of Observable.

Another discussion topic that came up was strategies for composing operators. Right now it's possible to compose operators onto an Observable instance using the lift method. The current RxJS design is prototype-based, so monkey-patching or extending are the way to add default operators.

Random notes, I know, but wanted to get them out there before I forget.

Did I miss or misrepresent anything @blesh @robwormald @IgorMinar?

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 5, 2015

Contributor

So to be clear, the lift method allows you to use an Operator from RxJS, which is a Subscriber factory to chain operators, but it's very unergonomic to do so.

One could either:

  1. Decorate the prototype
  2. Subclass Observable, overriding lift to return the Subclass, and add whatever operators you care about to that.
  3. Use ES7 function bind, which is available in Babel, but not TypeScript.

I think if you had an ObservableFactory create your Observable class for you, you could have that be responsible for decorating the Observable.prototype. I don't know enough about Angular's current DI, but I presume it can dynamically create classes to be injected, and not just object instances?

Contributor

benlesh commented Oct 5, 2015

So to be clear, the lift method allows you to use an Operator from RxJS, which is a Subscriber factory to chain operators, but it's very unergonomic to do so.

One could either:

  1. Decorate the prototype
  2. Subclass Observable, overriding lift to return the Subclass, and add whatever operators you care about to that.
  3. Use ES7 function bind, which is available in Babel, but not TypeScript.

I think if you had an ObservableFactory create your Observable class for you, you could have that be responsible for decorating the Observable.prototype. I don't know enough about Angular's current DI, but I presume it can dynamically create classes to be injected, and not just object instances?

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 5, 2015

Contributor

I don't know enough about Angular's current DI, but I presume it can dynamically create classes to be injected, and not just object instances?

Yes, toValue wouldn't necessarily be the strategy used. There is also toClass, toFactory, toAlias...maybe others.

Contributor

jeffbcross commented Oct 5, 2015

I don't know enough about Angular's current DI, but I presume it can dynamically create classes to be injected, and not just object instances?

Yes, toValue wouldn't necessarily be the strategy used. There is also toClass, toFactory, toAlias...maybe others.

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 6, 2015

Contributor

I just I just uglified and gzipped the dist/global/Rx.min.js bundle, and it is 37KB.

Contributor

jeffbcross commented Oct 6, 2015

I just I just uglified and gzipped the dist/global/Rx.min.js bundle, and it is 37KB.

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 6, 2015

Contributor

I browserified, uglified, and gzipped just the Observable using dist/cjs/Observable.js, and it is 4KB.

Contributor

jeffbcross commented Oct 6, 2015

I browserified, uglified, and gzipped just the Observable using dist/cjs/Observable.js, and it is 4KB.

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 6, 2015

Contributor

In the short-term, this essentially comes down to a decision between a user experience trade-off between two competing priorities:

  • Option 1: Include Observable + core operators that are included in Rx.js.
    • Benefit: User automatically gets all the operators they'll likely want
    • Cost:
      • 33KB extra in the core
      • For users who really don't want those bytes, they would need to rewrite import paths as part of a build process or loader configuration. Though, users who care this much are probably the types of users who are comfortable writing custom build processes and loader configurations.
  • Option 2: Include only Observable, only with absolute minimum operators (subscribe/foreach)
    • Benefit: 33K fewer bytes
    • Cost:
      • If a consumer wants to use Observable+core operators, they would have to re-cast any angular-exported observable to the the standard Rx observable using Rx.from(someAngularObservable);
      • We could use DI to allow overriding an ObservableFactory binding, so that users could bring-their-own-observable for Angular to use. But this would require a user to know about this binding, and require them to add the binding when bootstrapping their application
      • We could suggest that users simply mutate the "global" Observable prototype to include the operators they'd like. I believe this behavior already occurs by merely importing Rx.js, according to @vsavkin. This is generally considered a Bad Idea™, and goes against the ethos of Angular 2.
      • Typing information would be lost when replacing the plain Observable with a different-shaped Observable. The user would have to explicitly re-cast every Observable exported by Angular every place an Angular Observable is being consumed.

I personally favor Option 1, but I don't have as much context as @IgorMinar or @mhevery as to byte budgets.

The longer term approach is to focus on simplifying distribution and composition of operators, as well as helping to move forward standards that make this nicer (i.e. function bind).

Contributor

jeffbcross commented Oct 6, 2015

In the short-term, this essentially comes down to a decision between a user experience trade-off between two competing priorities:

  • Option 1: Include Observable + core operators that are included in Rx.js.
    • Benefit: User automatically gets all the operators they'll likely want
    • Cost:
      • 33KB extra in the core
      • For users who really don't want those bytes, they would need to rewrite import paths as part of a build process or loader configuration. Though, users who care this much are probably the types of users who are comfortable writing custom build processes and loader configurations.
  • Option 2: Include only Observable, only with absolute minimum operators (subscribe/foreach)
    • Benefit: 33K fewer bytes
    • Cost:
      • If a consumer wants to use Observable+core operators, they would have to re-cast any angular-exported observable to the the standard Rx observable using Rx.from(someAngularObservable);
      • We could use DI to allow overriding an ObservableFactory binding, so that users could bring-their-own-observable for Angular to use. But this would require a user to know about this binding, and require them to add the binding when bootstrapping their application
      • We could suggest that users simply mutate the "global" Observable prototype to include the operators they'd like. I believe this behavior already occurs by merely importing Rx.js, according to @vsavkin. This is generally considered a Bad Idea™, and goes against the ethos of Angular 2.
      • Typing information would be lost when replacing the plain Observable with a different-shaped Observable. The user would have to explicitly re-cast every Observable exported by Angular every place an Angular Observable is being consumed.

I personally favor Option 1, but I don't have as much context as @IgorMinar or @mhevery as to byte budgets.

The longer term approach is to focus on simplifying distribution and composition of operators, as well as helping to move forward standards that make this nicer (i.e. function bind).

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 6, 2015

Contributor

I believe this behavior already occurs by merely importing Rx.js, according to @vsavkin. This is generally considered a Bad Idea™, and goes against the ethos of Angular 2.

PRs welcome™

I'm in favor of Option 1, as well. And I'm willing to work with the community of RxJS and Angular to determine which operators should and should not be "core".

Also that would enable RxJS to put all core operators directly on Observable, which helps a few other issues.

Contributor

benlesh commented Oct 6, 2015

I believe this behavior already occurs by merely importing Rx.js, according to @vsavkin. This is generally considered a Bad Idea™, and goes against the ethos of Angular 2.

PRs welcome™

I'm in favor of Option 1, as well. And I'm willing to work with the community of RxJS and Angular to determine which operators should and should not be "core".

Also that would enable RxJS to put all core operators directly on Observable, which helps a few other issues.

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 6, 2015

Contributor

PRs welcome™

@blesh sorry, that wasn't meant to be a dig on RxJS, I was referring to the idea of mutating some shared global object not being an approach we should recommend.

Contributor

jeffbcross commented Oct 6, 2015

PRs welcome™

@blesh sorry, that wasn't meant to be a dig on RxJS, I was referring to the idea of mutating some shared global object not being an approach we should recommend.

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Oct 6, 2015

Member

I think we should thing more about Option #2:

  • Adding (mutating) global state for the purposes of code loading is OK (data is bad)
  • I believe there is a way to extend the .d.ts file with another file so that the typings are preserved. So no casting should be necessary.
  • 33K is a lot!
Member

mhevery commented Oct 6, 2015

I think we should thing more about Option #2:

  • Adding (mutating) global state for the purposes of code loading is OK (data is bad)
  • I believe there is a way to extend the .d.ts file with another file so that the typings are preserved. So no casting should be necessary.
  • 33K is a lot!
@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 6, 2015

Contributor

@mhevery what is "not a lot"? 20K? 15K? 10K?

Contributor

benlesh commented Oct 6, 2015

@mhevery what is "not a lot"? 20K? 15K? 10K?

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Oct 6, 2015

Member

to put bytes / size in context: ng1 is around 50k (min + gzip), react weights around 30k. I'm not sure if we are talking about 33k min + gzip here but if so it would be a lot in my books as well :-)

Member

pkozlowski-opensource commented Oct 6, 2015

to put bytes / size in context: ng1 is around 50k (min + gzip), react weights around 30k. I'm not sure if we are talking about 33k min + gzip here but if so it would be a lot in my books as well :-)

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Oct 6, 2015

Member

Based on @jeffbcross's comments (e.g. #4390 (comment)), it is indeed minified + gzipped.
But equally important is how much does ng2 "weight" (minified + gzipped).

Member

gkalpak commented Oct 6, 2015

Based on @jeffbcross's comments (e.g. #4390 (comment)), it is indeed minified + gzipped.
But equally important is how much does ng2 "weight" (minified + gzipped).

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 6, 2015

Contributor

One thing to know, the current "minified" version of RxJS 5 is not very minified. I don't think it's even mangling variable names. Could probably cut that down quite a bit.

Contributor

benlesh commented Oct 6, 2015

One thing to know, the current "minified" version of RxJS 5 is not very minified. I don't think it's even mangling variable names. Could probably cut that down quite a bit.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 6, 2015

Contributor

What is Angular 2 min & gzipped?

Contributor

benlesh commented Oct 6, 2015

What is Angular 2 min & gzipped?

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 6, 2015

Contributor

FWIW, if I run the current output Rx.js file through Closure Compiler it's 23.44KB gzipped

... I'm sure we can probably cut another ~3-5K out of that here or there.

Contributor

benlesh commented Oct 6, 2015

FWIW, if I run the current output Rx.js file through Closure Compiler it's 23.44KB gzipped

... I'm sure we can probably cut another ~3-5K out of that here or there.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 6, 2015

Contributor

... In fact, I bet a lot of that is just repetitious cruft from transpiling classes and modules.

Contributor

benlesh commented Oct 6, 2015

... In fact, I bet a lot of that is just repetitious cruft from transpiling classes and modules.

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Oct 7, 2015

Member

ng2 core bundle is currently ~110K, including Rx. But my understanding is that the goal is to get this number down to < 70K (and still keeping current size of ng1 as the number we should aim for)

Member

pkozlowski-opensource commented Oct 7, 2015

ng2 core bundle is currently ~110K, including Rx. But my understanding is that the goal is to get this number down to < 70K (and still keeping current size of ng1 as the number we should aim for)

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 7, 2015

Contributor

You couldn't get there if you completely removed Rx. It's only 23k right now. I suspect we could get it to 18k or so... But still.

Contributor

benlesh commented Oct 7, 2015

You couldn't get there if you completely removed Rx. It's only 23k right now. I suspect we could get it to 18k or so... But still.

@robwormald

This comment has been minimized.

Show comment
Hide comment
@robwormald

robwormald Oct 7, 2015

Member

Seems like maybe bytes isn't a great thing to base this decision off anyway, as the ideal case is some-tool (a la rollup) stripping out unused code / modules anyway in the not-too-distant future, so its hard to quantify what "Rx" or "Angular2" actually weighs?

On Oct 6, 2015, at 11:49 PM, Ben Lesh notifications@github.com wrote:

You couldn't get there if you completely removed Rx. It's only 23k right now. I suspect we could get it to 18k or so... But still.


Reply to this email directly or view it on GitHub #4390 (comment).

Member

robwormald commented Oct 7, 2015

Seems like maybe bytes isn't a great thing to base this decision off anyway, as the ideal case is some-tool (a la rollup) stripping out unused code / modules anyway in the not-too-distant future, so its hard to quantify what "Rx" or "Angular2" actually weighs?

On Oct 6, 2015, at 11:49 PM, Ben Lesh notifications@github.com wrote:

You couldn't get there if you completely removed Rx. It's only 23k right now. I suspect we could get it to 18k or so... But still.


Reply to this email directly or view it on GitHub #4390 (comment).

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 8, 2015

Contributor

@robwormald AFAIK rollup only works to tree-shake never-imported exports from ES6 modules. It would require some refactoring for Rx to take advantage of this (and angular, too, I'm sure) since operators are on the prototype.

Contributor

jeffbcross commented Oct 8, 2015

@robwormald AFAIK rollup only works to tree-shake never-imported exports from ES6 modules. It would require some refactoring for Rx to take advantage of this (and angular, too, I'm sure) since operators are on the prototype.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Oct 8, 2015

Contributor

We'd be better off getting ES7 function bind into TypeScript.

... although using ES7 function is likely a hit to operator performance I would guess. But probably nothing most people care about.

Contributor

benlesh commented Oct 8, 2015

We'd be better off getting ES7 function bind into TypeScript.

... although using ES7 function is likely a hit to operator performance I would guess. But probably nothing most people care about.

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross Oct 13, 2015

Contributor

Moving this into 41, working on a simple solution to manually add the combinators we need to EventEmitter without changing anything about its inheritance.

Contributor

jeffbcross commented Oct 13, 2015

Moving this into 41, working on a simple solution to manually add the combinators we need to EventEmitter without changing anything about its inheritance.

@jeffbcross jeffbcross modified the milestones: alpha-41, alpha-42 Oct 13, 2015

@robwormald robwormald modified the milestones: alpha-41, alpha-42 Oct 13, 2015

@jeffbcross jeffbcross closed this Oct 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment