Typeahead modifies Observable prototype

[achimha]

Bug description:

The current code modifies the prototype of Observable in the typeahead component using statements like here:

ng-bootstrap/src/typeahead/typeahead.ts

Line 21 in cfbc24a

import 'rxjs/add/observable/fromEvent';

import 'rxjs/add/observable/fromEvent';

This is bad for several reasons. The consuming application is not aware of what is imported where and under which conditions. Also it constitutes a side effect which is generally to be avoided. The right way to do it is to import the operator as a function and use it standalone (e.g. map.call(myObservable, ...)), passing the Observable as an argument. An example can be found in the Angular code base:

https://github.com/angular/angular/blob/master/modules/%40angular/router/src/router.ts#L20

https://github.com/angular/angular/blob/master/modules/%40angular/router/src/router.ts#L685

[pkozlowski-opensource]

@achimha thnx for reporting. While I agree on the principle I would like to hear more about the exact practical problems that this is causing in your case (to properly determine priority). Would you mind elaborating.

Also I'm not sure changing this will change much in practice.

The consuming application is not aware of what is imported where and under which conditions.

This won't change even if we change code as you suggest...

Also it constitutes a side effect which is generally to be avoided.

Once again, agree on the principle but currently any operator in RxJS works this way.

I'm trying to do a "good" thing here but would like to make sure that we are solving a real, practical problem.

[achimha]

I had a very unpleasant bug in my application where it would mysteriously throw a runtime exception when run on IE11 due to a missing operator. It turned out that I had forgotten to import the RxJS operator but a 3rd party library did, although not under all circumstances! This is just an example of a side-effect that can bite you.

Once again, agree on the principle but currently any operator in RxJS works this way.

I went through all 3rd party libraries I depend on to look for how widespread the problem is and while it is a common problem, only some libraries do it. Angular itself is very careful to not do it. RxJS offers each operator in a prototype-changing-add-fashion and as a separate function in the typical ES6 import fashion.

I do believe it is a practical problem, a library should not have side-effects, it defeats the purpose of the whole module/ES6 import paradigm.

The fix is very simple and ng-bootstrap only has it in one component so if you prefer I could try my luck with a PR...

[DzmitryShylovich]

The fix is very simple and ng-bootstrap only has it in one component so if you prefer I could try my luck with a PR...

it's not true https://github.com/ng-bootstrap/ng-bootstrap/search?utf8=%E2%9C%93&q=rxjs%2Fadd%2Fobservable

[pkozlowski-opensource]

it's not true https://github.com/ng-bootstrap/ng-bootstrap/search?utf8=%E2%9C%93&q=rxjs%2Fadd%2Fobservable

@DzmitryShylovich the other occurrences are in demo site, which is like an app, so IMO we are fine here. I hope that you are not suggesting that we can't extend Observable prototype at all?

[DzmitryShylovich]

he other occurrences are in demo site

yeah, really, sorry didn't notice that :)

[DzmitryShylovich]

Already asked Rob that question https://gitter.im/angular/angular?at=585322f4589f411830ed2746

importing angular shouldn't have side effects on the observable prototype
imagine one day we decide to switch something interally in the router and don't use map, so we remove it
then your code breaks, and you've got no idea why
you import methods on the prototype in your app
that's what you're supposed to do
(and that works fine)

[benlesh]

In Javascript, it's unfortunately pretty common to leak internal implementation details to consumers. This is generally done for performance or maintainability reasons within the library.

There are a few solutions for this:

1. The library author uses all operators directly, by importing them and using `operator.call(obs$, arg1, arg1)
2. The library consumer defensively has their own Observable build with prototype addition, etc, and assumes nothing is on the Observable. (Slightly more dangerous)
3. We get the function bind operator and the entire world is a better place.
4. Library authors use Symbols to decorate the prototypes manually so they can compose without trampling anything.

The RxJS team is keenly aware of this problem, and we're trying to come up with solid solutions. There have been a few proposals. (The Observable.prototype.op method is probably the most interesting, but the issue never got any traction)

[DzmitryShylovich]

@Blesh

The Observable.prototype.op method is probably the most interesting

link?

[pkozlowski-opensource]

Thnx @Blesh for chiming in. Based on all the discussion here I guess it won't do any harm if we use operators directly.

The fix is very simple and ng-bootstrap only has it in one component so if you prefer I could try my luck with a PR...

@achimha fancy a PR?

[benlesh]

@DzmitryShylovich oops sorry, here's the link... and it was the .o operator.

ReactiveX/rxjs#2034

[DzmitryShylovich]

@Blesh thx :)