feat(rxjs): fix #830, monkey patch rxjs to make rxjs run in correct zone #843

JiaLiPassion · 2017-07-16T05:12:45Z

copy logic from @mhevery 's PR on rxjs repository, ReactiveX/rxjs#2266, try to monkey patch rxjs to make

Observable construct
Subscription
Operator
unsubscribe

run in correct zone.

now the test can pass in Firefox 54 and node.

@mhevery , please review the idea is workable or not.

currently the risky part is
https://github.com/angular/zone.js/pull/843/files#diff-7dba82bf33d9e5b20f59836ce1601b82R25
To patch Observable constructor.

const Observable = rxjs.Observable;

rxjs.Observable = function() {
  Observable.apply(this, arguments);
  this._zone = Zone.current;
  return this;
};

I am not sure patch like this is ok or not. I will continue to find some other way to make sure new Observable() will use this patched version.

If the idea is ok, I will add other test cases and make it work for other browsers.
Thank you!

mleibman · 2017-07-17T05:03:34Z

lib/rxjs/rxjs.ts

+                return tearDownLogic.apply(this, arguments);
+              }
+            };
+            return patchedTeadDownLogic;


Typo in 'patchedTeadDownLogic'.

@mleibman, thanks!

mhevery · 2017-07-17T21:44:43Z

gulpfile.js

@@ -199,6 +199,14 @@ gulp.task('build/sync-test.js', ['compile-esm'], function(cb) {
  return generateScript('./lib/zone-spec/sync-test.ts', 'sync-test.js', false, cb);
 });

+gulp.task('build/rxjs.js', ['compile-esm'], function(cb) {
+  return generateScript('./lib/rxjs/rxjs.ts', 'zone-rxjs.js', false, cb);


Maybe we should call it zone-patch-rxjs to be consistent with zone-patch-cordova?

Got it, I will change it.

mhevery · 2017-07-17T22:41:20Z

lib/rxjs/rxjs.ts

+  const unsubscribeSource = 'rxjs.Subscriber.unsubscribe';
+
+  try {
+    rxjs = require('rxjs');


require statement will not work in browser.

So far the .js files have been stand alone code which can execute in global context, this means that we can't rely on ES6 import to get a hold of Rx.

There is also the matter that we have to run the code at a specific time. We have to load Zone.js first than Rx and only than we can do Rx patching but before the other code gets it.

I think the solution here is to package this file as umd that way it can be used in both cases.
It would mean that at some point one would have to import it as: zone.js/rxjs-patch

got it, I will try to organize the file into umd way.

mhevery · 2017-07-17T22:42:25Z

lib/rxjs/rxjs.ts

+      const _subscribe = this._subscribe;
+      if (_zone) {
+        this._subscribe = function() {
+          const subscriber = arguments.length > 0 ? arguments[0] : undefined;


referring to arguments is slower. Can we find another way to do this?

yeah, I will use an array to get the object.

mhevery · 2017-07-17T22:43:01Z

lib/rxjs/rxjs.ts

+    if (this.operator && _zone && _zone !== currentZone) {
+      const call = this.operator.call;
+      this.operator.call = function() {
+        const subscriber = arguments.length > 0 ? arguments[0] : undefined;


don't refer to arguments

yeah, I will use an array to get the object.

JiaLiPassion · 2017-07-19T13:10:33Z

@mhevery , I have modified the code based on your comment, and the travis build will be passed after #844 was merged, please review.

And I found current patch only handle the basic methods and operators of Observable (such as subscribe, lift), and the other methods and operators which are not included in Observable are not patched, such as Observable.bindCallback, The reason current patch not work is, for example, bindCallback's implementation like this,

// BoundCallbackObservable
static create<T>(func: Function,
                   selector: Function | void = undefined,
                   scheduler?: IScheduler): (...args: any[]) => Observable<T> {
    return function(this: any, ...args: any[]): Observable<T> {
      return new BoundCallbackObservable<T>(func, <any>selector, args, this, scheduler);
    };
  }

Observable.bindCallback =  BoundCallbackObservable.create

And BoundCallbackObservable constructor look like this,

// BoundCallbackObservable extends Observable
constructor(private callbackFunc: Function,
              private selector: Function,
              private args: any[],
              private context: any,
              private scheduler: IScheduler) {
    super();
  }

And the super is unpatched Observable, so the ConstructorZone is not captured.

I just added a new function and test cases to also patch the case like this, and I will continue to
patch other methods and operators which also inherit the Observable class.

Finally, all Observable creation operation should be patched.

…rrect zone

mhevery

I think fixing the import/UMD is the only thing which is left.

mhevery · 2017-07-19T22:52:32Z

test/rxjs/rxjs.spec.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+let rxjs: any;


Why can't we do proper import {Rx} from 'rxjs/Rx';? I think that should work.

I found a way to make import work in both

npm run test (local build env)

travis karma jasmine (dist env)

So we can use import * as Rx from 'rxjs/Rx' in rxjs.ts and rxjs.spec.ts now, please review.

mhevery · 2017-07-19T23:26:47Z

lib/rxjs/rxjs.ts

+ */
+
+declare let define: any;
+(function(root: any, factory: (Rx: any) => any) {


Instead of doing it this way we should use rollup umd function to build the UMD.

This should be replaced with simple import {RX} from 'rxjs'

See: https://rollupjs.org/repl?version=0.45.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QlJ4JTdEJTIwZnJvbSUyMCdyeGpzJTJGUlgnJTVDbiU1Q24lMkYlMkYlMjBkbyUyMHNvbWV0aGluZyUyMCUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJ1bWQlMjIlMkMlMjJtb2R1bGVOYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTJDJTIybW9kdWxlSWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

I changed it into import * as Rx from 'rxjs/Rx'

mhevery · 2017-07-19T23:40:55Z

lib/rxjs/rxjs.ts

+  (Rx: any) => {
+    'use strict';
+    Zone.__load_patch('rxjs', (global: any, Zone: ZoneType, api: _ZonePrivate) => {
+      const subscribeSource = 'rxjs.subscribe';


Out of these consts only nextSource seems to be used.

I have modified this one.

JiaLiPassion · 2017-07-20T04:07:05Z

@misko, I just updated the import/UMD part.

update test/main.ts, if we run in local build env, we just let systemjs load Rx.js for us.
add a global-rxjs.ts, if we run in travis dist env, we will load dist/zone-patch-rxjs.js which is an umd module, and will patch window.Rx, so we can't let systemjs to load Rx.js otherwise it will load another copy, so I add global-rxjs.ts to let systemjs load the object from window.Rx.
Now the karma test will work in both local build and travis dist env.
zone-patch-rxjs.js is built by rollup as an umd module.

Please review.

misko · 2017-07-20T17:10:40Z

@mhevery

googlebot · 2017-07-20T20:58:07Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-07-20T20:58:07Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot added the cla: yes label Jul 16, 2017

mleibman reviewed Jul 17, 2017

View reviewed changes

mhevery suggested changes Jul 17, 2017

View reviewed changes

JiaLiPassion force-pushed the rxjs branch 11 times, most recently from bf5b906 to e90479a Compare July 19, 2017 04:07

JiaLiPassion added 10 commits July 20, 2017 01:50

feat(rxjs): fix angular#830, monkey patch rxjs to make rxjs run in co…

d37e332

…rrect zone

make test pass

0afdb96

add karma config

9b651e1

pass compile-esm

093ffb7

change rxjs to umd

b129e41

change spec to pass test

2fc5d46

add comment, patch Observable.create

da92051

refactor, not create a new closure _subscribe everytime

0e5ad22

fix set wrong object

f56c213

begin to apply observable methods, 1: bindCallback

b238b09

JiaLiPassion force-pushed the rxjs branch from f9bab1f to b238b09 Compare July 19, 2017 16:51

fix async scheduler reset zone issue

3889379

mhevery suggested changes Jul 19, 2017

View reviewed changes

change require to import

eaebc3a

JiaLiPassion added 5 commits July 20, 2017 13:28

still need require in rxjs.spec

f855927

make build and dist pass test

1b4d50c

remove meta def

18d9965

make import work on both build and dist

389472e

continue to add cases

696f6cb

mhevery approved these changes Jul 20, 2017

View reviewed changes

Merge branch 'master' into rxjs

e754d4d

googlebot added cla: no and removed cla: yes labels Jul 20, 2017

mhevery merged commit 1ed83d0 into angular:master Jul 20, 2017

JiaLiPassion deleted the rxjs branch July 20, 2017 22:29

ccrowhurstram mentioned this pull request Jul 31, 2017

Error thrown by rxjs observer not always sent to ErrorHandler.handleError angular/angular#14316

Closed

JiaLiPassion mentioned this pull request Sep 14, 2017

fix(core): guarantee ngZone.onStable/onError run outside of ngZone angular/angular#19192

Closed

3 tasks

trotyl mentioned this pull request Jun 15, 2018

Observable from window resize event is ignored by change detection in Internet Explorer angular/angular#9060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rxjs): fix #830, monkey patch rxjs to make rxjs run in correct zone #843

feat(rxjs): fix #830, monkey patch rxjs to make rxjs run in correct zone #843

JiaLiPassion commented Jul 16, 2017 •

edited

Loading

mleibman Jul 17, 2017

JiaLiPassion Jul 17, 2017

mhevery Jul 17, 2017

JiaLiPassion Jul 18, 2017

mhevery Jul 17, 2017

JiaLiPassion Jul 18, 2017

mhevery Jul 17, 2017

JiaLiPassion Jul 18, 2017

mhevery Jul 17, 2017

JiaLiPassion Jul 18, 2017

JiaLiPassion commented Jul 19, 2017 •

edited

Loading

mhevery left a comment

mhevery Jul 19, 2017

JiaLiPassion Jul 19, 2017 •

edited

Loading

mhevery Jul 19, 2017

JiaLiPassion Jul 19, 2017 •

edited

Loading

mhevery Jul 19, 2017

JiaLiPassion Jul 19, 2017 •

edited

Loading

JiaLiPassion commented Jul 20, 2017 •

edited

Loading

misko commented Jul 20, 2017

googlebot commented Jul 20, 2017

googlebot commented Jul 20, 2017

feat(rxjs): fix #830, monkey patch rxjs to make rxjs run in correct zone #843

feat(rxjs): fix #830, monkey patch rxjs to make rxjs run in correct zone #843

Conversation

JiaLiPassion commented Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion commented Jul 19, 2017 • edited Loading

mhevery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

JiaLiPassion commented Jul 20, 2017 • edited Loading

misko commented Jul 20, 2017

googlebot commented Jul 20, 2017

googlebot commented Jul 20, 2017

JiaLiPassion commented Jul 16, 2017 •

edited

Loading

JiaLiPassion commented Jul 19, 2017 •

edited

Loading

JiaLiPassion Jul 19, 2017 •

edited

Loading

JiaLiPassion Jul 19, 2017 •

edited

Loading

JiaLiPassion Jul 19, 2017 •

edited

Loading

JiaLiPassion commented Jul 20, 2017 •

edited

Loading