Skip to content

Commit

Permalink
refactor: avoid polluting the user's rxjs setup
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
crisbeto committed Jan 31, 2017
1 parent 08e9d70 commit ff4ba5f
Show file tree
Hide file tree
Showing 18 changed files with 296 additions and 141 deletions.
90 changes: 58 additions & 32 deletions CODING_STANDARDS.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ ES6 or TypeScript.
### General

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

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

For example, this is a not-very-useful comment:
Expand Down Expand Up @@ -55,22 +55,22 @@ do this:
```

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

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

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

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

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

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

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

#### Fluent APIs
When creating a fluent or builder-pattern style API, use the `this` return type for methods:
```
```ts
class ConfigBuilder {
withName(name: string): this {
this.config.name = name;
Expand All @@ -95,13 +95,39 @@ class ConfigBuilder {
}
```

#### RxJS
When dealing with RxJS operators, import the operator functions directly (e.g.
`import "rxjs/operator/map"`), as opposed to using the "patch" imports which pollute the user's
global Observable object (e.g. `import "rxjs/add/operator/map"`):

```ts
// NO
import 'rxjs/add/operator/map';
someObservable.map(...).subscribe(...);

// YES
import {map} from 'rxks/operator/map';
map.call(someObservable, ...).subscribe(...);
```

Note that this approach can be inflexible when dealing with long chains of operators. You can use
the `FunctionChain` class to help with it:

```ts
// Before
someObservable.filter(...).map(...).do(...);

// After
new FunctionChain(someObservable).call(filter, ...).call(map, ...).call(do, ...).execute();
```

#### Access modifiers
* Omit the `public` keyword as it is the default behavior.
* Use `private` when appropriate and possible, prefixing the name with an underscore.
* Use `protected` when appropriate and possible with no prefix.
* Prefix *library-internal* properties and methods with an underscore without using the `private`
* Prefix *library-internal* properties and methods with an underscore without using the `private`
keyword. This is necessary for anything that must be public (to be used by Angular), but should not
be part of the user-facing API. This typically applies to symbols used in template expressions,
be part of the user-facing API. This typically applies to symbols used in template expressions,
`@ViewChildren` / `@ContentChildren` properties, host bindings, and `@Input` / `@Output` properties
(when using an alias).

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

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

Properties should have a concise description of what the property means:
```ts
/** The label position relative to the checkbox. Defaults to 'after' */
@Input() labelPosition: 'before' | 'after' = 'after';
```
```

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

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

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

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


### CSS

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

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

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

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

```scss
the-host-element {
// ...

.some-child-element {
color: red;
}
Expand Down Expand Up @@ -267,4 +293,4 @@ When it is not super obvious, include a brief description of what a class repres

// Portion of the floating panel that sits, invisibly, on top of the input.
.md-datepicker-input-mask { }
```
```
34 changes: 19 additions & 15 deletions src/lib/autocomplete/autocomplete-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ import {NgControl} from '@angular/forms';
import {Overlay, OverlayRef, OverlayState, TemplatePortal} from '../core';
import {MdAutocomplete} from './autocomplete';
import {PositionStrategy} from '../core/overlay/position/position-strategy';
import {FunctionChain} from '../core/util/function-chain';
import {Observable} from 'rxjs/Observable';
import {MdOptionSelectEvent} from '../core/option/option';
import 'rxjs/add/observable/merge';
import {Dir} from '../core/rtl/dir';
import 'rxjs/add/operator/startWith';
import 'rxjs/add/operator/switchMap';
import {merge} from 'rxjs/observable/merge';
import {first} from 'rxjs/operator/first';
import {switchMap} from 'rxjs/operator/switchMap';
import {startWith} from 'rxjs/operator/startWith';


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

/** Stream of autocomplete option selections. */
Expand All @@ -84,17 +86,19 @@ export class MdAutocompleteTrigger implements OnDestroy {
* stream every time the option list changes.
*/
private _subscribeToClosingActions(): void {
// Every time the option list changes...
this.autocomplete.options.changes
// and also at initialization, before there are any option changes...
.startWith(null)
// create a new stream of panelClosingActions, replacing any previous streams
// that were created, and flatten it so our stream only emits closing events...
.switchMap(() => this.panelClosingActions)
// when the first closing event occurs...
.first()
// set the value, close the panel, and complete.
.subscribe(event => this._setValueAndClose(event));
new FunctionChain<Observable<any>>()
// Every time the option list changes...
.context(this.autocomplete.options.changes)
// and also at initialization, before there are any option changes...
.call(startWith, null)
// create a new stream of panelClosingActions, replacing any previous streams
// that were created, and flatten it so our stream only emits closing events...
.call(switchMap, () => this.panelClosingActions)
// when the first closing event occurs...
.call(first)
.execute()
// set the value, close the panel, and complete.
.subscribe(event => this._setValueAndClose(event));
}

/** Destroys the autocomplete suggestion panel. */
Expand Down
5 changes: 3 additions & 2 deletions src/lib/core/a11y/focus-trap.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {Component, ViewEncapsulation, ViewChild, ElementRef, Input, NgZone} from '@angular/core';
import {first} from 'rxjs/operator/first';
import {InteractivityChecker} from './interactivity-checker';
import {coerceBooleanProperty} from '../coercion/boolean-property';

Expand Down Expand Up @@ -33,7 +34,7 @@ export class FocusTrap {
* trap region.
*/
focusFirstTabbableElementWhenReady() {
this._ngZone.onMicrotaskEmpty.first().subscribe(() => {
first.call(this._ngZone.onMicrotaskEmpty).subscribe(() => {
this.focusFirstTabbableElement();
});
}
Expand All @@ -43,7 +44,7 @@ export class FocusTrap {
* trap region.
*/
focusLastTabbableElementWhenReady() {
this._ngZone.onMicrotaskEmpty.first().subscribe(() => {
first.call(this._ngZone.onMicrotaskEmpty).subscribe(() => {
this.focusLastTabbableElement();
});
}
Expand Down
3 changes: 2 additions & 1 deletion src/lib/core/a11y/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {FocusKeyManager} from './focus-key-manager';
import {DOWN_ARROW, UP_ARROW, TAB, HOME, END} from '../keyboard/keycodes';
import {ListKeyManager} from './list-key-manager';
import {ActiveDescendantKeyManager} from './activedescendant-key-manager';
import {first} from 'rxjs/operator/first';

class FakeFocusable {
disabled = false;
Expand Down Expand Up @@ -196,7 +197,7 @@ describe('Key managers', () => {

it('should emit tabOut when the tab key is pressed', () => {
let tabOutEmitted = false;
keyManager.tabOut.first().subscribe(() => tabOutEmitted = true);
first.call(keyManager.tabOut).subscribe(() => tabOutEmitted = true);
keyManager.onKeydown(TAB_EVENT);

expect(tabOutEmitted).toBe(true);
Expand Down
6 changes: 3 additions & 3 deletions src/lib/core/overlay/scroll/scroll-dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {Scrollable} from './scrollable';
import {Subject} from 'rxjs/Subject';
import {Observable} from 'rxjs/Observable';
import {Subscription} from 'rxjs/Subscription';
import 'rxjs/add/observable/fromEvent';
import {fromEvent} from 'rxjs/observable/fromEvent';


/**
Expand All @@ -23,8 +23,8 @@ export class ScrollDispatcher {

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

/**
Expand Down
4 changes: 2 additions & 2 deletions src/lib/core/overlay/scroll/scrollable.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {Directive, ElementRef, OnInit, OnDestroy} from '@angular/core';
import {Observable} from 'rxjs/Observable';
import {ScrollDispatcher} from './scroll-dispatcher';
import 'rxjs/add/observable/fromEvent';
import {fromEvent} from 'rxjs/observable/fromEvent';


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

getElementRef(): ElementRef {
Expand Down
46 changes: 46 additions & 0 deletions src/lib/core/util/function-chain.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import {FunctionChain} from './function-chain';

describe('FunctionChain', () => {
it('should chain functions', () => {
let first = jasmine.createSpy('First function');
let second = jasmine.createSpy('Second function');
let third = jasmine.createSpy('Third function');

new FunctionChain().call(first).call(second).call(third).execute();

expect(first).toHaveBeenCalled();
expect(second).toHaveBeenCalled();
expect(third).toHaveBeenCalled();
});

it('should pass in arguments to the functions', () => {
let first = jasmine.createSpy('First function');
let second = jasmine.createSpy('Second function');

new FunctionChain().call(first, 1, 2).call(second, 3, 4).execute();

expect(first).toHaveBeenCalledWith(1, 2);
expect(second).toHaveBeenCalledWith(3, 4);
});

it('should clear the chain once it has been executed', () => {
let fn = jasmine.createSpy('Spy function');
let chain = new FunctionChain().call(fn);

chain.execute();
expect(fn).toHaveBeenCalledTimes(1);

chain.execute();
expect(fn).toHaveBeenCalledTimes(1);
});

it('should return the final function result', () => {
let result = new FunctionChain()
.context([1, 2, 3, 4, 5])
.call(Array.prototype.map, (current: number) => current * 2)
.call(Array.prototype.filter, (current: number) => current > 5)
.execute();

expect(result).toEqual([6, 8, 10]);
});
});

0 comments on commit ff4ba5f

Please sign in to comment.