Refactor to use bare RxJS implementations. #5288

robwormald · 2015-11-14T08:23:07Z

change RxJS implementation to use bare Observable and Subject with no included operators.

jeffbcross · 2015-11-16T17:22:18Z

modules/playground/src/http/http_comp.ts

-    http.get('./people.json')
-        .map((res: Response) => res.json())
-        .subscribe((people: Array<Object>) => this.people = people);
+    http.get('./people.json').subscribe(res => { this.people = res.json(); });


Did this need to be changed as part of this PR?

Ah, I changed that when I did the first commit, I'll add it back.

Still needs to be fixed.

jeffbcross · 2015-11-16T18:58:29Z

Any reason why we can't just re-export Observable and add to the prototype? The only failure I see is the public API increasing because of all the static method stubs on Observable.

import {Observable} from '@reactivex/rxjs/dist/cjs/Observable';
export {Observable} from '@reactivex/rxjs/dist/cjs/Observable';

Observable.prototype.map = map;
Observable.prototype.filter = filter;

jeffbcross · 2015-11-16T19:20:47Z

Also, @vsavkin made the valid point that we should include flatMap since it accepts observables, and is a good basis to chain operators. To not have flatMap would be crippling, and I should have added it in the original issue :(.

I'm going to chat with @IgorMinar about this PR, because he said he has some thoughts.

jeffbcross · 2015-11-20T23:24:52Z

@robwormald LGTM once you remove the last commit and finish the cleanup comments.

You could also wait until ReactiveX/rxjs#751 and ReactiveX/rxjs#750 land if you'd like.

IgorMinar · 2015-11-21T10:37:55Z

modules/angular2/src/facade/async.ts

@@ -3,9 +3,18 @@ import {global, isPresent} from 'angular2/src/facade/lang';
 // without depending on rxjs.
 import {PromiseWrapper, Promise, PromiseCompleter} from 'angular2/src/facade/promise';


can you remove this unused import in a separate commit? I'd do it, but then I'd create a conflict for you

IgorMinar · 2015-11-21T10:41:20Z

the rest looks good

robwormald · 2015-11-21T10:50:28Z

ObservableWrapper.fromPromise / .toPromise are gonna fail now. Thoughts?

IgorMinar · 2015-11-21T10:51:36Z

let's go with barebones first and then create PRs that add these things and
measure payload diff. Then decide.

On Sat, Nov 21, 2015 at 2:50 AM Rob Wormald notifications@github.com
wrote:

ObservableWrapper.fromPromise / .toPromise are gonna fail now. Thoughts?

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

IgorMinar · 2015-11-21T10:52:06Z

barebones = what's in this PR now

On Sat, Nov 21, 2015 at 2:51 AM Igor Minar igor@angularjs.org wrote:

let's go with barebones first and then create PRs that add these things
and measure payload diff. Then decide.

On Sat, Nov 21, 2015 at 2:50 AM Rob Wormald notifications@github.com
wrote:

ObservableWrapper.fromPromise / .toPromise are gonna fail now. Thoughts?

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

bradlygreen · 2015-11-30T23:46:37Z

@robwormald CI is failing...can you take a look?

move to new RxJS distribution. BREAKING CHANGE: RxJS imports now are via `rxjs` instead of `@reactivex/rxjs` Individual operators can be imported `import 'rxjs/operators/map'`

robwormald · 2015-12-01T01:43:27Z

alllllright, big changes. new version of rxjs in, minimal API surface

toPromise() / fromPromise() cc @vsavkin
take() (only in mock_backend)

@jeffbcross @IgorMinar plz discuss

robwormald · 2015-12-01T01:47:25Z

will need to add following operators:

flatMap
map
from (spec)
of (spec)

pkozlowski-opensource · 2015-12-01T10:25:50Z

npm-shrinkwrap.clean.json

@@ -13268,33 +13268,6 @@
        }
      }
    },
-    "source-map-loader": {


Why is the source-map-loader removed? It looks like something went wrong with the shrinkwrap process (npm install not executed?)

pkozlowski-opensource · 2015-12-01T10:27:19Z

IMO shrinkwrap is broken in this PR. It needs to be corrected as it might break source maps for bundles.

jeffbcross · 2015-12-01T17:47:59Z

gulpfile.js

-  } else {
-    return source;
-  }
+  var n = source.indexOf('System.register("rxjs/Subject"');


We can't omit the license. What is the underlying problem?

It doesn't ship in the new repo. I can grab it from the old I guess.

jeffbcross · 2015-12-01T18:58:31Z

TODOs:

Add flatMap, map, from, of, filter, reduce operators
Measure difference in bundle size from bare Observable and Observable + ^^^ operators
Include rxjs in the benchpress bundle (it is required, and the previous exclude pattern doesn't seem to have actually been working)
Fix shrinkwrap to not remove sourcemap
Remove PromiseWrapper unused import in a separate commit
Restore E2E test to its former Rx-ified glory
Find a way to include the RxJS license

Regarding take in mock_backend, as long as we only import it in a testing barrel, no need to be concerned with added bytes.

I'll take care of all of the todos except the last one for now. @robwormald do you want to try to correct the license issue upstream and convince @Blesh to cut another RxJS release?

mprobst · 2015-12-01T19:03:49Z

@jeffbcross I filed an issue for the newline-at-EOF, I don't think that's supported yet.

jeffbcross · 2015-12-01T19:42:11Z

Thanks @mprobst!

benlesh · 2015-12-01T20:03:01Z

Find a way to include the RxJS license

Is this to say that the new packages are missing the LICENSE file?... that's not good. lol

I'll take care of all of the todos except the last one for now. @robwormald do you want to try to correct the license issue upstream and convince @Blesh to cut another RxJS release?

The beta release is coming this week (hopefully in a couple of days). Does it need to be before that?

robwormald · 2015-12-01T20:22:29Z

I'll fix the Rx stuff over there first, and in the interim can just grab
the license from the @ReactiveX package.
On Tue, Dec 1, 2015 at 12:03 PM Ben Lesh notifications@github.com wrote:

Find a way to include the RxJS license

Is this to say that the new packages are missing the LICENSE file?...
that's not good. lol

I'll take care of all of the todos except the last one for now.
@robwormald https://github.com/robwormald do you want to try to correct
the license issue upstream and convince @Blesh https://github.com/blesh
to cut another RxJS release?

The beta release is coming this week (hopefully in a couple of days). Does
it need to be before that?

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

jeffbcross · 2015-12-01T20:40:12Z

Bundle size measurements (using the reported numbers from gulp bundles.js.checksize for gzip level 2 as well as running gzip --best -c dist/build/angular2.min.js > dist/build/angular2.min.js.gz for gzip level 9).

variant	gzip level 2 (bytes)	gzip level 9 (bytes)
with no operators	153,670	118,022
with 8 operators*	160,233	123,755
difference from bare	-5563	-5733
with 4 operators**	156303	120,250
difference from bare	-2633	-2228

*these are the operators included in the "8 operators"

import 'rxjs/operators/map';
import 'rxjs/operators/mergeMap'; //aka flatMap
import 'rxjs/operators/filter';
import 'rxjs/operators/reduce';
import 'rxjs/operators/toPromise';

import 'rxjs/observable/from';
import 'rxjs/observable/fromArray'; //Observable.of
import 'rxjs/observable/fromPromise';

**these are the operators included in the "4 operators"

import 'rxjs/operators/map';
import 'rxjs/operators/mergeMap'; //aka flatMap
import 'rxjs/operators/filter';
import 'rxjs/operators/reduce';

CC @IgorMinar

benlesh · 2015-12-01T21:08:11Z

FWIW, Observable.of and Observable.from are part of the Observable spec.

jeffbcross · 2015-12-01T21:35:06Z

FYI I opened #5533 to fix some of the remaining issues here (everything except the license issue)

angular-automatic-lock-bot · 2019-09-07T15:20:39Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Nov 14, 2015

robwormald force-pushed the refactor/observable branch 4 times, most recently from c9e8ca4 to b4a430d Compare November 14, 2015 08:55

robwormald mentioned this pull request Nov 14, 2015

Observable Payload Reduction Plan #5283

Closed

robwormald force-pushed the refactor/observable branch from ddbc0a5 to b8ef694 Compare November 15, 2015 02:03

robwormald added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 16, 2015

robwormald assigned jeffbcross Nov 16, 2015

jeffbcross reviewed Nov 16, 2015
View reviewed changes

robwormald added this to the beta-00 milestone Nov 16, 2015

robwormald force-pushed the refactor/observable branch from a68fed5 to 12f9357 Compare November 20, 2015 01:52

jeffbcross added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 20, 2015

jeffbcross assigned robwormald and unassigned jeffbcross Nov 20, 2015

jeffbcross added the pr_state: LGTM label Nov 20, 2015

IgorMinar reviewed Nov 21, 2015
View reviewed changes

robwormald force-pushed the refactor/observable branch from 12f9357 to 93a35fa Compare November 21, 2015 10:49

robwormald added 3 commits November 30, 2015 17:37

chore(package): use ultralight ObservableS

f248c96

refactor(facade): use rxjs package

8933647

move to new RxJS distribution. BREAKING CHANGE: RxJS imports now are via `rxjs` instead of `@reactivex/rxjs` Individual operators can be imported `import 'rxjs/operators/map'`

refactor(http): remove mockbackend export

8bdc2cd

robwormald force-pushed the refactor/observable branch from 482f38f to 81a8602 Compare December 1, 2015 01:39

robwormald added action: review The PR is still awaiting reviews from at least one requested reviewer and removed pr_state: LGTM labels Dec 1, 2015

robwormald assigned IgorMinar and unassigned alxhub Dec 1, 2015

chore(package): use rxjs package

904b838

robwormald force-pushed the refactor/observable branch from 81a8602 to 904b838 Compare December 1, 2015 01:46

robwormald mentioned this pull request Dec 1, 2015

chore(build): enforce import checks for http #5510

Closed

pkozlowski-opensource reviewed Dec 1, 2015
View reviewed changes

jeffbcross reviewed Dec 1, 2015
View reviewed changes

jeffbcross mentioned this pull request Dec 1, 2015

Continuation of RxJS stripping #5533

Closed

7 tasks

jeffbcross closed this Dec 1, 2015

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to use bare RxJS implementations. #5288

Refactor to use bare RxJS implementations. #5288

robwormald commented Nov 14, 2015

jeffbcross Nov 16, 2015

robwormald Nov 16, 2015

jeffbcross Dec 1, 2015

jeffbcross commented Nov 16, 2015

jeffbcross commented Nov 16, 2015

jeffbcross commented Nov 20, 2015

IgorMinar Nov 21, 2015

IgorMinar commented Nov 21, 2015

robwormald commented Nov 21, 2015

IgorMinar commented Nov 21, 2015

IgorMinar commented Nov 21, 2015

bradlygreen commented Nov 30, 2015

robwormald commented Dec 1, 2015

robwormald commented Dec 1, 2015

pkozlowski-opensource Dec 1, 2015

pkozlowski-opensource commented Dec 1, 2015

jeffbcross Dec 1, 2015

robwormald Dec 1, 2015

jeffbcross commented Dec 1, 2015

mprobst commented Dec 1, 2015

jeffbcross commented Dec 1, 2015

benlesh commented Dec 1, 2015

robwormald commented Dec 1, 2015

jeffbcross commented Dec 1, 2015

benlesh commented Dec 1, 2015

jeffbcross commented Dec 1, 2015

angular-automatic-lock-bot bot commented Sep 7, 2019

		@@ -3,9 +3,18 @@ import {global, isPresent} from 'angular2/src/facade/lang';
		// without depending on rxjs.
		import {PromiseWrapper, Promise, PromiseCompleter} from 'angular2/src/facade/promise';

Refactor to use bare RxJS implementations. #5288

Refactor to use bare RxJS implementations. #5288

Conversation

robwormald commented Nov 14, 2015

jeffbcross Nov 16, 2015

Choose a reason for hiding this comment

robwormald Nov 16, 2015

Choose a reason for hiding this comment

jeffbcross Dec 1, 2015

Choose a reason for hiding this comment

jeffbcross commented Nov 16, 2015

jeffbcross commented Nov 16, 2015

jeffbcross commented Nov 20, 2015

IgorMinar Nov 21, 2015

Choose a reason for hiding this comment

IgorMinar commented Nov 21, 2015

robwormald commented Nov 21, 2015

IgorMinar commented Nov 21, 2015

IgorMinar commented Nov 21, 2015

bradlygreen commented Nov 30, 2015

robwormald commented Dec 1, 2015

robwormald commented Dec 1, 2015

pkozlowski-opensource Dec 1, 2015

Choose a reason for hiding this comment

pkozlowski-opensource commented Dec 1, 2015

jeffbcross Dec 1, 2015

Choose a reason for hiding this comment

robwormald Dec 1, 2015

Choose a reason for hiding this comment

jeffbcross commented Dec 1, 2015

mprobst commented Dec 1, 2015

jeffbcross commented Dec 1, 2015

benlesh commented Dec 1, 2015

robwormald commented Dec 1, 2015

jeffbcross commented Dec 1, 2015

benlesh commented Dec 1, 2015

jeffbcross commented Dec 1, 2015

angular-automatic-lock-bot bot commented Sep 7, 2019