Skip to content

Commit ff4ba5f

Browse files
committed
refactor: avoid polluting the user's rxjs setup
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 #2622.
1 parent 08e9d70 commit ff4ba5f

18 files changed

+296
-141
lines changed

CODING_STANDARDS.md

Lines changed: 58 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@ ES6 or TypeScript.
1212
### General
1313

1414
#### Write useful comments
15-
Comments that explain what some block of code does are nice; they can tell you something in less
15+
Comments that explain what some block of code does are nice; they can tell you something in less
1616
time than it would take to follow through the code itself.
1717

18-
Comments that explain why some block of code exists at all, or does something the way it does,
19-
are _invaluable_. The "why" is difficult, or sometimes impossible, to track down without seeking out
20-
the original author. When collaborators are in the same room, this hurts productivity.
18+
Comments that explain why some block of code exists at all, or does something the way it does,
19+
are _invaluable_. The "why" is difficult, or sometimes impossible, to track down without seeking out
20+
the original author. When collaborators are in the same room, this hurts productivity.
2121
When collaborators are in different timezones, this can be devastating to productivity.
2222

2323
For example, this is a not-very-useful comment:
@@ -55,22 +55,22 @@ do this:
5555
```
5656

5757
#### Prefer small, focused modules
58-
Keeping modules to a single responsibility makes the code easier to test, consume, and maintain.
59-
ES6 modules offer a straightforward way to organize code into logical, granular units.
58+
Keeping modules to a single responsibility makes the code easier to test, consume, and maintain.
59+
ES6 modules offer a straightforward way to organize code into logical, granular units.
6060
Ideally, individual files are 200 - 300 lines of code.
6161

62-
As a rule of thumb, once a file draws near 400 lines (barring abnormally long constants / comments),
63-
start considering how to refactor into smaller pieces.
62+
As a rule of thumb, once a file draws near 400 lines (barring abnormally long constants / comments),
63+
start considering how to refactor into smaller pieces.
6464

6565
#### Less is more
66-
Once a feature is released, it never goes away. We should avoid adding features that don't offer
67-
high user value for price we pay both in maintenance, complexity, and payload size. When in doubt,
68-
leave it out.
66+
Once a feature is released, it never goes away. We should avoid adding features that don't offer
67+
high user value for price we pay both in maintenance, complexity, and payload size. When in doubt,
68+
leave it out.
6969

70-
This applies especially so to providing two different APIs to accomplish the same thing. Always
70+
This applies especially so to providing two different APIs to accomplish the same thing. Always
7171
prefer sticking to a _single_ API for accomplishing something.
7272

73-
### 100 column limit
73+
### 100 column limit
7474
All code and docs in the repo should be 100 columns or fewer. This applies to TypeScript, SCSS,
7575
HTML, bash scripts, and markdown files.
7676

@@ -81,12 +81,12 @@ Avoid `any` where possible. If you find yourself using `any`, consider whether a
8181
appropriate in your case.
8282

8383
For methods and properties that are part of a component's public API, all types must be explicitly
84-
specified because our documentation tooling cannot currently infer types in places where TypeScript
84+
specified because our documentation tooling cannot currently infer types in places where TypeScript
8585
can.
8686

8787
#### Fluent APIs
8888
When creating a fluent or builder-pattern style API, use the `this` return type for methods:
89-
```
89+
```ts
9090
class ConfigBuilder {
9191
withName(name: string): this {
9292
this.config.name = name;
@@ -95,13 +95,39 @@ class ConfigBuilder {
9595
}
9696
```
9797

98+
#### RxJS
99+
When dealing with RxJS operators, import the operator functions directly (e.g.
100+
`import "rxjs/operator/map"`), as opposed to using the "patch" imports which pollute the user's
101+
global Observable object (e.g. `import "rxjs/add/operator/map"`):
102+
103+
```ts
104+
// NO
105+
import 'rxjs/add/operator/map';
106+
someObservable.map(...).subscribe(...);
107+
108+
// YES
109+
import {map} from 'rxks/operator/map';
110+
map.call(someObservable, ...).subscribe(...);
111+
```
112+
113+
Note that this approach can be inflexible when dealing with long chains of operators. You can use
114+
the `FunctionChain` class to help with it:
115+
116+
```ts
117+
// Before
118+
someObservable.filter(...).map(...).do(...);
119+
120+
// After
121+
new FunctionChain(someObservable).call(filter, ...).call(map, ...).call(do, ...).execute();
122+
```
123+
98124
#### Access modifiers
99125
* Omit the `public` keyword as it is the default behavior.
100126
* Use `private` when appropriate and possible, prefixing the name with an underscore.
101127
* Use `protected` when appropriate and possible with no prefix.
102-
* Prefix *library-internal* properties and methods with an underscore without using the `private`
128+
* Prefix *library-internal* properties and methods with an underscore without using the `private`
103129
keyword. This is necessary for anything that must be public (to be used by Angular), but should not
104-
be part of the user-facing API. This typically applies to symbols used in template expressions,
130+
be part of the user-facing API. This typically applies to symbols used in template expressions,
105131
`@ViewChildren` / `@ContentChildren` properties, host bindings, and `@Input` / `@Output` properties
106132
(when using an alias).
107133

@@ -114,14 +140,14 @@ All public APIs must have user-facing comments. These are extracted and shown in
114140
on [material.angular.io](https://material.angular.io).
115141

116142
Private and internal APIs should have JsDoc when they are not obvious. Ultimately it is the purview
117-
of the code reviwer as to what is "obvious", but the rule of thumb is that *most* classes,
118-
properties, and methods should have a JsDoc description.
143+
of the code reviwer as to what is "obvious", but the rule of thumb is that *most* classes,
144+
properties, and methods should have a JsDoc description.
119145

120146
Properties should have a concise description of what the property means:
121147
```ts
122148
/** The label position relative to the checkbox. Defaults to 'after' */
123149
@Input() labelPosition: 'before' | 'after' = 'after';
124-
```
150+
```
125151

126152
Methods blocks should describe what the function does and provide a description for each parameter
127153
and the return value:
@@ -145,7 +171,7 @@ Boolean properties and return values should use "Whether..." as opposed to "True
145171

146172
##### General
147173
* Prefer writing out words instead of using abbreviations.
148-
* Prefer *exact* names over short names (within reason). E.g., `labelPosition` is better than
174+
* Prefer *exact* names over short names (within reason). E.g., `labelPosition` is better than
149175
`align` because the former much more exactly communicates what the property means.
150176
* Except for `@Input` properties, use `is` and `has` prefixes for boolean properties / methods.
151177

@@ -169,25 +195,25 @@ The name of a method should capture the action that is performed *by* that metho
169195
### Angular
170196

171197
#### Host bindings
172-
Prefer using the `host` object in the directive configuration instead of `@HostBinding` and
173-
`@HostListener`. We do this because TypeScript preserves the type information of methods with
198+
Prefer using the `host` object in the directive configuration instead of `@HostBinding` and
199+
`@HostListener`. We do this because TypeScript preserves the type information of methods with
174200
decorators, and when one of the arguments for the method is a native `Event` type, this preserved
175-
type information can lead to runtime errors in non-browser environments (e.g., server-side
176-
pre-rendering).
201+
type information can lead to runtime errors in non-browser environments (e.g., server-side
202+
pre-rendering).
177203

178204

179205
### CSS
180206

181207
#### Be cautious with use of `display: flex`
182-
* The [baseline calculation for flex elements](http://www.w3.org/TR/css-flexbox-1/#flex-baselines)
183-
is different than other display values, making it difficult to align flex elements with standard
208+
* The [baseline calculation for flex elements](http://www.w3.org/TR/css-flexbox-1/#flex-baselines)
209+
is different than other display values, making it difficult to align flex elements with standard
184210
elements like input and button.
185211
* Component outermost elements are never flex (block or inline-block)
186212
* Don't use `display: flex` on elements that will contain projected content.
187213

188214
#### Use lowest specificity possible
189-
Always prioritize lower specificity over other factors. Most style definitions should consist of a
190-
single element or css selector plus necessary state modifiers. **Avoid SCSS nesting for the sake of
215+
Always prioritize lower specificity over other factors. Most style definitions should consist of a
216+
single element or css selector plus necessary state modifiers. **Avoid SCSS nesting for the sake of
191217
code organization.** This will allow users to much more easily override styles.
192218

193219
For example, rather than doing this:
@@ -224,12 +250,12 @@ md-calendar {
224250
The end-user of a component should be the one to decide how much margin a component has around it.
225251

226252
#### Prefer styling the host element vs. elements inside the template (where possible).
227-
This makes it easier to override styles when necessary. For example, rather than
253+
This makes it easier to override styles when necessary. For example, rather than
228254

229255
```scss
230256
the-host-element {
231257
// ...
232-
258+
233259
.some-child-element {
234260
color: red;
235261
}
@@ -267,4 +293,4 @@ When it is not super obvious, include a brief description of what a class repres
267293

268294
// Portion of the floating panel that sits, invisibly, on top of the input.
269295
.md-datepicker-input-mask { }
270-
```
296+
```

src/lib/autocomplete/autocomplete-trigger.ts

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@ import {NgControl} from '@angular/forms';
55
import {Overlay, OverlayRef, OverlayState, TemplatePortal} from '../core';
66
import {MdAutocomplete} from './autocomplete';
77
import {PositionStrategy} from '../core/overlay/position/position-strategy';
8+
import {FunctionChain} from '../core/util/function-chain';
89
import {Observable} from 'rxjs/Observable';
910
import {MdOptionSelectEvent} from '../core/option/option';
10-
import 'rxjs/add/observable/merge';
1111
import {Dir} from '../core/rtl/dir';
12-
import 'rxjs/add/operator/startWith';
13-
import 'rxjs/add/operator/switchMap';
12+
import {merge} from 'rxjs/observable/merge';
13+
import {first} from 'rxjs/operator/first';
14+
import {switchMap} from 'rxjs/operator/switchMap';
15+
import {startWith} from 'rxjs/operator/startWith';
1416

1517

1618
/** The panel needs a slight y-offset to ensure the input underline displays. */
@@ -70,7 +72,7 @@ export class MdAutocompleteTrigger implements OnDestroy {
7072
*/
7173
get panelClosingActions(): Observable<any> {
7274
// TODO(kara): add tab event observable with keyboard event PR
73-
return Observable.merge(...this.optionSelections, this._overlayRef.backdropClick());
75+
return merge.call(Observable, ...this.optionSelections, this._overlayRef.backdropClick());
7476
}
7577

7678
/** Stream of autocomplete option selections. */
@@ -84,17 +86,19 @@ export class MdAutocompleteTrigger implements OnDestroy {
8486
* stream every time the option list changes.
8587
*/
8688
private _subscribeToClosingActions(): void {
87-
// Every time the option list changes...
88-
this.autocomplete.options.changes
89-
// and also at initialization, before there are any option changes...
90-
.startWith(null)
91-
// create a new stream of panelClosingActions, replacing any previous streams
92-
// that were created, and flatten it so our stream only emits closing events...
93-
.switchMap(() => this.panelClosingActions)
94-
// when the first closing event occurs...
95-
.first()
96-
// set the value, close the panel, and complete.
97-
.subscribe(event => this._setValueAndClose(event));
89+
new FunctionChain<Observable<any>>()
90+
// Every time the option list changes...
91+
.context(this.autocomplete.options.changes)
92+
// and also at initialization, before there are any option changes...
93+
.call(startWith, null)
94+
// create a new stream of panelClosingActions, replacing any previous streams
95+
// that were created, and flatten it so our stream only emits closing events...
96+
.call(switchMap, () => this.panelClosingActions)
97+
// when the first closing event occurs...
98+
.call(first)
99+
.execute()
100+
// set the value, close the panel, and complete.
101+
.subscribe(event => this._setValueAndClose(event));
98102
}
99103

100104
/** Destroys the autocomplete suggestion panel. */

src/lib/core/a11y/focus-trap.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {Component, ViewEncapsulation, ViewChild, ElementRef, Input, NgZone} from '@angular/core';
2+
import {first} from 'rxjs/operator/first';
23
import {InteractivityChecker} from './interactivity-checker';
34
import {coerceBooleanProperty} from '../coercion/boolean-property';
45

@@ -33,7 +34,7 @@ export class FocusTrap {
3334
* trap region.
3435
*/
3536
focusFirstTabbableElementWhenReady() {
36-
this._ngZone.onMicrotaskEmpty.first().subscribe(() => {
37+
first.call(this._ngZone.onMicrotaskEmpty).subscribe(() => {
3738
this.focusFirstTabbableElement();
3839
});
3940
}
@@ -43,7 +44,7 @@ export class FocusTrap {
4344
* trap region.
4445
*/
4546
focusLastTabbableElementWhenReady() {
46-
this._ngZone.onMicrotaskEmpty.first().subscribe(() => {
47+
first.call(this._ngZone.onMicrotaskEmpty).subscribe(() => {
4748
this.focusLastTabbableElement();
4849
});
4950
}

src/lib/core/a11y/list-key-manager.spec.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {FocusKeyManager} from './focus-key-manager';
33
import {DOWN_ARROW, UP_ARROW, TAB, HOME, END} from '../keyboard/keycodes';
44
import {ListKeyManager} from './list-key-manager';
55
import {ActiveDescendantKeyManager} from './activedescendant-key-manager';
6+
import {first} from 'rxjs/operator/first';
67

78
class FakeFocusable {
89
disabled = false;
@@ -196,7 +197,7 @@ describe('Key managers', () => {
196197

197198
it('should emit tabOut when the tab key is pressed', () => {
198199
let tabOutEmitted = false;
199-
keyManager.tabOut.first().subscribe(() => tabOutEmitted = true);
200+
first.call(keyManager.tabOut).subscribe(() => tabOutEmitted = true);
200201
keyManager.onKeydown(TAB_EVENT);
201202

202203
expect(tabOutEmitted).toBe(true);

src/lib/core/overlay/scroll/scroll-dispatcher.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {Scrollable} from './scrollable';
33
import {Subject} from 'rxjs/Subject';
44
import {Observable} from 'rxjs/Observable';
55
import {Subscription} from 'rxjs/Subscription';
6-
import 'rxjs/add/observable/fromEvent';
6+
import {fromEvent} from 'rxjs/observable/fromEvent';
77

88

99
/**
@@ -23,8 +23,8 @@ export class ScrollDispatcher {
2323

2424
constructor() {
2525
// By default, notify a scroll event when the document is scrolled or the window is resized.
26-
Observable.fromEvent(window.document, 'scroll').subscribe(() => this._notify());
27-
Observable.fromEvent(window, 'resize').subscribe(() => this._notify());
26+
fromEvent.call(Observable, window.document, 'scroll').subscribe(() => this._notify());
27+
fromEvent.call(Observable, window, 'resize').subscribe(() => this._notify());
2828
}
2929

3030
/**

src/lib/core/overlay/scroll/scrollable.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import {Directive, ElementRef, OnInit, OnDestroy} from '@angular/core';
22
import {Observable} from 'rxjs/Observable';
33
import {ScrollDispatcher} from './scroll-dispatcher';
4-
import 'rxjs/add/observable/fromEvent';
4+
import {fromEvent} from 'rxjs/observable/fromEvent';
55

66

77
/**
@@ -28,7 +28,7 @@ export class Scrollable implements OnInit, OnDestroy {
2828
* Returns observable that emits when a scroll event is fired on the host element.
2929
*/
3030
elementScrolled(): Observable<any> {
31-
return Observable.fromEvent(this._elementRef.nativeElement, 'scroll');
31+
return fromEvent.call(Observable, this._elementRef.nativeElement, 'scroll');
3232
}
3333

3434
getElementRef(): ElementRef {
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import {FunctionChain} from './function-chain';
2+
3+
describe('FunctionChain', () => {
4+
it('should chain functions', () => {
5+
let first = jasmine.createSpy('First function');
6+
let second = jasmine.createSpy('Second function');
7+
let third = jasmine.createSpy('Third function');
8+
9+
new FunctionChain().call(first).call(second).call(third).execute();
10+
11+
expect(first).toHaveBeenCalled();
12+
expect(second).toHaveBeenCalled();
13+
expect(third).toHaveBeenCalled();
14+
});
15+
16+
it('should pass in arguments to the functions', () => {
17+
let first = jasmine.createSpy('First function');
18+
let second = jasmine.createSpy('Second function');
19+
20+
new FunctionChain().call(first, 1, 2).call(second, 3, 4).execute();
21+
22+
expect(first).toHaveBeenCalledWith(1, 2);
23+
expect(second).toHaveBeenCalledWith(3, 4);
24+
});
25+
26+
it('should clear the chain once it has been executed', () => {
27+
let fn = jasmine.createSpy('Spy function');
28+
let chain = new FunctionChain().call(fn);
29+
30+
chain.execute();
31+
expect(fn).toHaveBeenCalledTimes(1);
32+
33+
chain.execute();
34+
expect(fn).toHaveBeenCalledTimes(1);
35+
});
36+
37+
it('should return the final function result', () => {
38+
let result = new FunctionChain()
39+
.context([1, 2, 3, 4, 5])
40+
.call(Array.prototype.map, (current: number) => current * 2)
41+
.call(Array.prototype.filter, (current: number) => current > 5)
42+
.execute();
43+
44+
expect(result).toEqual([6, 8, 10]);
45+
});
46+
});

0 commit comments

Comments
 (0)