Skip to content
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

Internal usages of RxJS operators should not patch the prototype of Observable #2622

Closed
robwormald opened this issue Jan 12, 2017 · 3 comments · Fixed by #5314
Closed

Internal usages of RxJS operators should not patch the prototype of Observable #2622

robwormald opened this issue Jan 12, 2017 · 3 comments · Fixed by #5314
Assignees

Comments

@robwormald
Copy link
Contributor

robwormald commented Jan 12, 2017

Bug, feature request, or proposal:

All internal usages of Observable / RxJS operators should use the non-patch syntax for imports.

What is the expected behavior?

Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland:

//desired syntax
import {Observable} from 'rxjs/Observable'
import {map} from 'rxjs/operator/map'

map.call(sourceObservable, (x) => x + 1).subscribe(...)

What is the current behavior?

//current syntax
import {Observable} from 'rxjs/Observable'
import 'rxjs/add/operator/map'

sourceObservable.map((x) => x + 1).subscribe(...)

What is the use-case or motivation for changing an existing behavior?

Currently the "patch" operators are used, which pollute the Observable prototype and can lead to unexpected results if, for example, Material uses the .map() operator today, and then removes it in the future. If a user uses .map() without explicitly importing it, their app would break upon removal by Material.

Which versions of Angular, Material, OS, browsers are affected?

Is there anything else we should know?

Sample of correct usage in Router : https://github.com/angular/angular/blob/52be848f94a70feffd44a1d688286f750d0471d9/modules/%40angular/router/src/router_config_loader.ts#L34

@crisbeto crisbeto self-assigned this Jan 13, 2017
@crisbeto
Copy link
Member

crisbeto commented Jan 13, 2017

@robwormald how do you recommend handling the longer chains of calls? We have some more complex cases like https://github.com/angular/material2/blob/master/src/lib/icon/icon-registry.ts#L248

CC @jelbourn

crisbeto added a commit to crisbeto/material2 that referenced this issue Jan 14, 2017
Currently, the internal uses of RxJS operators are done via patch imports, which add methods directly to the global `Observable` object. This can be an issue since the user may not know where these methods are coming from and may depend on them. Afterwards, if Material removes a method import, the user's app will break since the methods won't be defined anymore. This PR:

* Replaces all of the patch imports with imports of individual operators and fixes any issues that showed up afterwards.
* Adds a `FunctionChain` class to help with chaining the individually-imported operators.

Fixes angular#2622.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jan 19, 2017
Currently, the internal uses of RxJS operators are done via patch imports, which add methods directly to the global `Observable` object. This can be an issue since the user may not know where these methods are coming from and may depend on them. Afterwards, if Material removes a method import, the user's app will break since the methods won't be defined anymore. This PR:

* Replaces all of the patch imports with imports of individual operators and fixes any issues that showed up afterwards.
* Adds a `FunctionChain` class to help with chaining the individually-imported operators.

Fixes angular#2622.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jan 31, 2017
Currently, the internal uses of RxJS operators are done via patch imports, which add methods directly to the global `Observable` object. This can be an issue since the user may not know where these methods are coming from and may depend on them. Afterwards, if Material removes a method import, the user's app will break since the methods won't be defined anymore. This PR:

* Replaces all of the patch imports with imports of individual operators and fixes any issues that showed up afterwards.
* Adds a `FunctionChain` class to help with chaining the individually-imported operators.

Fixes angular#2622.
@crisbeto crisbeto removed the has pr label Jun 21, 2017
crisbeto added a commit to crisbeto/material2 that referenced this issue Jun 22, 2017
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly and chaining them via the `RxChain` class.

Fixes angular#2622.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jun 22, 2017
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly and chaining them via the `RxChain` class.

Fixes angular#2622.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jun 23, 2017
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly and chaining them via the `RxChain` class.

Fixes angular#2622.
jelbourn pushed a commit to crisbeto/material2 that referenced this issue Jun 23, 2017
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly and chaining them via the `RxChain` class.

Fixes angular#2622.
jelbourn pushed a commit that referenced this issue Jun 23, 2017
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly and chaining them via the `RxChain` class.

Fixes #2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 17, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 17, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 17, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 18, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 18, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 19, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 19, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 20, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 20, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 20, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 20, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 20, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 20, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 27, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 27, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
ThomasBurleson added a commit to angular/flex-layout that referenced this issue Jul 27, 2017
Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.
andrewseguin pushed a commit to angular/flex-layout that referenced this issue Jul 27, 2017
* fix(build): sync to ngM2 build processes

* lint and style fixes
* tools and package fixes
* npm process improvements
* support aot, build, test, lint, demo-app tasks
* demo css refactors, lint fixes
* stage-deploy:devapp now deploys aot version

* feat(lib): remove uses of rxjs patch operators

Usages of RxJS operators should use the "longhand" syntax, which avoids polluting the Observable.prototype in userland.
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly.

> Refs angular/components#2622.

* chore(demo-app): no longer fallback to index.html file

Gulp Connect currently always falls back to the index.html file if a resource could not be found. This is not a good idea because this way the browser or Angular is never able to report not-found errors.

* chore(build): more changes base on PR feedback

*  artifact scripts should push empty commits
   > When publishing build artifacts (cdk; material) there will be always a change (regardless if something changed in the package or not). This is because the version will be always appended with the current SHA. But when publishing the docs content this is not the case. Sometimes there won't be any change and Git will exit with error code 1 because empty commits are not allowed.
*  remove unused docs, screenshot, and dashboard tools
@mgechev
Copy link
Member

mgechev commented Oct 12, 2017

Here's a tslint rule I implemented some time back, based on a discussion we had with @hansl, which could be used as prevention mechanism of dynamic behavior which won't be caught by the type system.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants