fix(rxjs): fix #863, fix asap scheduler issue, add testcases

[JiaLiPassion]

fix #863.

1. Rx.scheduler.asap will not use window.setImmediate, instead it use window.postMessage, and the postMessage eventListener is a global function which was bound when application bootup, so the function will always run in <root> zone, in this PR, we patch asap scheduler to run in correct zone.

2. continue to add testcases. ( in progress)

current progress.

• Observable static factory methods.
• Observable.prototype.methods

3. What is patched and why.

• Rx.Observable

Patch Observable constructor. Keep Zone.current.

new Rx.Observable((_subscriber: any) => {
      subscriber = _subscriber;
      log.push('setup');
      expect(Zone.current.name).toEqual(constructorZone.name);
      return () => {
        expect(Zone.current.name).toEqual(constructorZone.name);
        log.push('cleanup');
   };

So _subscriber which passed into constructor and tearDownLogic will run in the Zone when create Observable

• Rx.Observable.lift

patch operator

• Rx.Observable.create

patch static factory method , So _subscriber which passed into constructor and tearDownLogic will run in the Zone when create Observable

• Rx.Observable static methods.

  • bindCallback, the callback should be called in the Zone when the callback was bound.
  • bindNodeCallback, the same with bindCallback
  • combineLatest, don't need to be patched. It was called by lift.
  • concat, don't need to be patched, It was called by lift.
  • defer, the defer function was patched. Because the callback will be called later.
  • empty, don't need to be patched.
  • forkJoin, need to be patched, because it support forkJoin(observable, selector) signature, the
    selector function also need to be called in the Zone it was created.
  • from, don't need to be patched.
  • fromEvent, don't need to be patched, because addEventListener/removeEventListener already
    be patched by zone.js.
  • fromEventPattern, need to be patched, because it will pass addListener/removeListener
    callback need to be run in the Zone when it was created.
  • fromPromise, don't need to be patched, the promise already been patched.
  • interval, don't need to be patched, setInterval already been patched.
  • merge, don't need to be patched, It was called by lift.
  • never, don't need to be patched.
  • of, don't need to be patched, ArrayObservable don't need to be patched, because it don't accept
    _subscribe in constructor.
  • range, don't need to be patched, ArrayObservable don't need to be patched, because it don't
    accept _subscribe in constructor.
  • throw, don't need to be patched.
  • timer, don't need to be patched. setInterval has been patched.
  • webSocket, don't need to be patched, webSocket has been patched.
  • zip, don't need to be patched, It was called by lift.

• Rx.Observable.prototype methods
  basically all operator methods, don't need to be patched, It was called by lift.

• Rx.Observable.multicast methods
  multicast have subscriberFactory, it should run in the Zone which was created.

• Rx.Scheduler.asap
  patch asap because it not use setImmediate, it use window.postMessage.

• Rx.Observable._subscribe
  have been patched to make sure subscriber callback run in the Zone when observable.subscribe was called.

[JiaLiPassion]

@mhevery , I have finished adding test cases for basically all the APIs of rxjs 5.

• some API need to be patched, some don't. I have write which were patched and why above.
• current patch use import * as Rx from 'rxjs/RX', it will load everything from rxjs, it will have performance impact, I am not sure angular treeshaking will handle this and only bundle the used operators?
• another issue is , if user import Observable in this way import {Observable} from 'rxjs/Observable', the Observable is still non patched, if angular use webpack, we may cheat webpack to load patched Observable.

Please review.

[mhevery]

@benlesh could you have a look?

[mhevery]

current patch use import * as Rx from 'rxjs/RX', it will load everything from rxjs, it will have performance impact, I am not sure angular treeshaking will handle this and only bundle the used operators?

This is an issue as we are moving in the direction of individual tree shaking.

[mhevery]

One thing which is not clear to me is why do we need to batch the scheduler? Could you give me an example where non patched scheduler will do the wrong thing?

[JiaLiPassion]

@mhevery , about the Rx.Scheduler.asap, the following case will not work without patch.

   let func;
   constructorZone.run(() => {   
       func = function(arg0: any, callback: Function) {
           expect(Zone.current.name).toEqual(constructorZone.name);
           callback(arg0);
         };
         boundFunc = Rx.Observable.bindCallback(func, null, Rx.Scheduler.asap);
         observable = boundFunc('test');
   });
       
   observable.subscribe(...)

Because Rx.Scheduler.asap not using setImmediate but window.addEventListener('message', handler) + window.postMessage, and the handler is a global one which is initialized when bootup, so it will always be <root> zone.

[JiaLiPassion]

@mhevery , I have updated the code and the umd build.

now rxjs patch will only import several necessary libraries which are listed in the bottom of this comment.

Please review.

What have been patched.

Below is the summarize of what have been patched.

1. Patch Observable to make sure everything created when constructing Observable will run in constructor zone.

• patch Observable.prototype._subscribe setter for new Observable(_subscribe) case.

   const observable: any = constructorZone.run(() => new Rx.Observable((_subscriber: any) => {
      expect(Zone.current.name).toEqual(constructorZone.name);
    }));

• patch Observable.create for Observable.create(_subscribe) case.

  let observable: any = constructorZone.run(() => Rx.Observable.create((subscriber: any) => {
             expect(Zone.current.name).toEqual(constructorZone.name);
    }));

• patch Observable.prototype.source for Observable.lift() case to handle all operators

  Observable.of(1,2).map(item => {
         expect(Zone.current.name).toEqual(constructorZone.name);
   });

• patch Observable.subscribe to handle tearDownLogic case

  const observable: any = constructorZone.run(() => new Rx.Observable((_subscriber: any) => {
      return () => {
        expect(Zone.current.name).toEqual(constructorZone.name);
      };
    }));

2. patch Subscriber and Subscription prototype's next/error/complete/unsubscribe.

3. Some function which was passed when constructing Observable and were executed when subscribe observable should also be patched to make sure they are run in constructor zone not subscribe zone.

• defer
• bindCallback
• bindNodeCallback
• forkJoin
• fromEventPattern
• multicast

4. patch Immediate of Rx.Scheduler.asap, Rx.Scheduler.asap use Rx.Scheduler.Immediate which not using setImmediate if window.postMessage is available. And rxjs will register a global handler for window.on('message') which make the global event handler always run in root zone, so we also need to patch Immediate.

5. For UMD build, now we build a stand alone zone-patch-rxjs.js which is an umd format library.
   and only import

import 'rxjs/add/observable/bindCallback';
import 'rxjs/add/observable/bindNodeCallback';
import 'rxjs/add/observable/defer';
import 'rxjs/add/observable/forkJoin';
import 'rxjs/add/observable/fromEventPattern';
import 'rxjs/add/operator/multicast';

import {Observable} from 'rxjs/Observable';
import {asap} from 'rxjs/scheduler/asap';
import {Subscriber} from 'rxjs/Subscriber';
import {Subscription} from 'rxjs/Subscription';

those libraries.

[JiaLiPassion]

@mhevery , I updated again.

• basic patch use this one, this is more clearer.
  https://github.com/trxcllnt/rxjs/blob/53eddb1cca1228fe6d81aa6bec0ee0a57c8f4664/src/add/zone.ts

1. Observable.prototype._subscribe as a setter to save constructor zone.
2. Observable.prototype.source as a setter to save constructor zone in operator.
3. Observable.prototype.subscribe to run _subscribe or operator.call in constructor zone.
4. Subscription.prototype._unsubscribe as a setter to save subscription zone.
5. Subscriber.prototype.destination as a setter to save subscription.
6. Subscriber.next/error/complete to run next/error/complete in subscription zone.
7. Subscription.prototype.unsubscribe to run unsubscribe in constructor zone.

• and add Observable.defer/fromEventPattern/bindCallback/bindNodeCallback/multicast and Immediate patch.

• update import, add

import {rxSubscriber} from 'rxjs/symbol/rxSubscriber';

the reason is we need this symbol to implement toSubscribe ourselves, the toSubscribe method
is not exported in Rx.js umd module.

please review.

[mhevery]

I don't understand why do we need this file. Could you provide some insight? This file seems to just reexport things which means that it should be unnecessary? Could you explain what issue you are trying to solve?

[JiaLiPassion]

@mhevery , yes, you are right, in the previous version, we patch Observable constructor, so I need this to make sure in karma dist test run, only one rxjs is loaded, now we don't patch Observable constructor any longer, we don't need this one. I will remove it.

Updated

in the previous commit, I add a global-rxjs.ts, the purpose is, in karma dist test, the load order is

1. load node_modules/rxjs/bundles/Rx.js, this is an umd module.
2. load dist/zone-patch-rxjs.js, this is also an umd module.

when load dist/zone-patch-rxjs.js, it will try to import operators such as import 'rxjs/add/observable/bindCallback';, because we use systemjs, it will try to load rxjs/add/observable/bindCallback.js from karma server. So I have to map rxjs/add/observable/bindCallback to somewhere, and make sure it use the same module as node_modules/rxjs/bundles/Rx.js, so I map it to a global-rxjs which only do an re-export.

I don’t know if I describe it clearly, please check it.

[mhevery]

Why do we need this? Why can't we just use standard way of loading RxJS?

[JiaLiPassion]

The same as above, I will remove it.

[mhevery]

It looks like we went from sytemJS loading the code to UMD. Why do we need to do this change? I think SystemJS was fine.

[JiaLiPassion]

@mhevery , yes, the same reason with above comment, I will change it.

[JiaLiPassion]

@mhevery , I have updated the source to remove unnecessary karma rxjs loader code , please review.

##Updated##

sorry, I did not use zone-patch-rxjs.js in karma dist test, I will modify it.