Address jelbourn's review comments

Author: yifange

src/cdk-experimental/selection/BUILD.bazel

@@ -1,7 +1,6 @@
 package(default_visibility = ["//visibility:public"])
 
-load("//src/e2e-app:test_suite.bzl", "e2e_test_suite")
-load("//tools:defaults.bzl", "ng_e2e_test_library", "ng_module", "ng_test_library", "ng_web_test_suite")
+load("//tools:defaults.bzl", "ng_module")
 
 ng_module(
     name = "selection",

src/cdk-experimental/selection/row-selection.ts

@@ -6,7 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Directive, HostBinding, Input} from '@angular/core';
+import {coerceNumberProperty, NumberInput} from '@angular/cdk/coercion';
+import {Directive, Input} from '@angular/core';
 
 import {CdkSelection} from './selection';
 
@@ -19,31 +20,24 @@ import {CdkSelection} from './selection';
  */
 @Directive({
   selector: '[cdkRowSelection]',
+  host: {
+    '[class.cdk-selected]': '_selection.isSelected(this._value, this._index)',
+    '[attr.aria-selected]': '_selection.isSelected(this._value, this._index)',
+  },
 })
 export class CdkRowSelection<T> {
-  @Input()
-  get cdkRowSelectionValue(): T {
-    return this._value;
-  }
-  set cdkRowSelectionValue(value: T) {
-    this._value = value;
-  }
-  _value: T;
+  @Input('cdkRowSelectionValue') _value: T;
 
-  @Input()
-  get cdkRowSelectionIndex(): number|undefined {
+  @Input('cdkRowSelectionIndex')
+  get index(): number|undefined {
     return this._index;
   }
-  set cdkRowSelectionIndex(index: number|undefined) {
-    this._index = index;
+  set index(index: number|undefined) {
+    this._index = coerceNumberProperty(index);
   }
   _index?: number;
 
-  constructor(private readonly _selection: CdkSelection<T>) {}
+  constructor(readonly _selection: CdkSelection<T>) {}
 
-  @HostBinding('class.cdk-selected')
-  @HostBinding('attr.aria-selected')
-  get isSelected() {
-    return this._selection.isSelected(this._value, this._index);
-  }
+  static ngAcceptInputType_index: NumberInput;
 }

src/cdk-experimental/selection/select-all.ts

@@ -6,9 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Directive, Inject, OnDestroy, OnInit, Optional, Self} from '@angular/core';
+import {Directive, Inject, isDevMode, OnDestroy, OnInit, Optional, Self} from '@angular/core';
 import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
-import {BehaviorSubject, of as observableOf, Subject} from 'rxjs';
+import {Observable, of as observableOf, ReplaySubject} from 'rxjs';
 import {switchMap, takeUntil} from 'rxjs/operators';
 
 import {CdkSelection} from './selection';
@@ -32,14 +32,18 @@ export class CdkSelectAll<T> implements OnDestroy, OnInit {
    * The checked state of the toggle.
    * Resolves to `true` if all the values are selected, `false` if no value is selected.
    */
-  readonly checked$ = new BehaviorSubject(false);
+  readonly checked: Observable<boolean> = this._selection.change.pipe(
+      switchMap(() => observableOf(this._selection.isAllSelected())),
+  );
 
   /**
    * The indeterminate state of the toggle.
    * Resolves to `true` if part (not all) of the values are selected, `false` if all values or no
    * value at all are selected.
    */
-  readonly indeterminate$ = new BehaviorSubject(false);
+  readonly indeterminate: Observable<boolean> = this._selection.change.pipe(
+      switchMap(() => observableOf(this._selection.isPartialSelected())),
+  );
 
   /**
    * Toggles the select-all state.
@@ -60,55 +64,43 @@ export class CdkSelectAll<T> implements OnDestroy, OnInit {
     });
   }
 
-  private readonly _destroyed$ = new Subject();
+  private readonly _destroyed = new ReplaySubject(1);
 
   constructor(
       @Optional() private readonly _selection: CdkSelection<T>,
       @Optional() @Self() @Inject(NG_VALUE_ACCESSOR) private readonly _controlValueAccessor:
           ControlValueAccessor[]) {}
 
   ngOnInit() {
-    if (!this._selection) {
-      throw new Error('CdkSelectAll: missing CdkSelection in the parent');
-    }
-
-    if (!this._selection.cdkSelectionMultiple) {
-      throw new Error('CdkSelectAll: CdkSelection must have cdkSelectionMultiple set to true');
-    }
-
-    this._selection.cdkSelectionChange
-        .pipe(
-            switchMap(() => observableOf(this._selection.isAllSelected())),
-            takeUntil(this._destroyed$),
-            )
-        .subscribe((state) => {
-          this.checked$.next(state);
-        });
-
-    this._selection.cdkSelectionChange
-        .pipe(
-            switchMap(() => observableOf(this._selection.isPartialSelected())),
-            takeUntil(this._destroyed$),
-            )
-        .subscribe((state) => {
-          this.indeterminate$.next(state);
-        });
+    this._assertValidParentSelection();
+    this._configureControlValueAccessor();
+  }
 
+  private _configureControlValueAccessor() {
     if (this._controlValueAccessor && this._controlValueAccessor.length) {
       this._controlValueAccessor[0].registerOnChange((e: unknown) => {
         if (e === true || e === false) {
           this.toggle();
         }
       });
-
-      this.checked$.pipe(takeUntil(this._destroyed$)).subscribe((state) => {
+      this.checked.pipe(takeUntil(this._destroyed)).subscribe((state) => {
         this._controlValueAccessor[0].writeValue(state);
       });
     }
   }
 
+  private _assertValidParentSelection() {
+    if (!this._selection && isDevMode()) {
+      throw Error('CdkSelectAll: missing CdkSelection in the parent');
+    }
+
+    if (!this._selection.multiple && isDevMode()) {
+      throw Error('CdkSelectAll: CdkSelection must have cdkSelectionMultiple set to true');
+    }
+  }
+
   ngOnDestroy() {
-    this._destroyed$.next();
-    this._destroyed$.complete();
+    this._destroyed.next();
+    this._destroyed.complete();
   }
 }

src/cdk-experimental/selection/selection-column.ts

@@ -10,6 +10,7 @@ import {CdkCellDef, CdkColumnDef, CdkHeaderCellDef, CdkTable} from '@angular/cdk
 import {
   Component,
   Input,
+  isDevMode,
   OnDestroy,
   OnInit,
   Optional,
@@ -29,11 +30,11 @@ import {CdkSelection} from './selection';
   template: `
     <ng-container cdkColumnDef>
       <th cdkHeaderCell *cdkHeaderCellDef>
-        <input type="checkbox" *ngIf="selection.cdkSelectionMultiple"
+        <input type="checkbox" *ngIf="selection.multiple"
             cdkSelectAll
             #allToggler="cdkSelectAll"
-            [checked]="allToggler.checked$ | async"
-            [indeterminate]="allToggler.indeterminate$ | async"
+            [checked]="allToggler.checked | async"
+            [indeterminate]="allToggler.indeterminate | async"
             (click)="allToggler.toggle($event)">
       </th>
       <td cdkCell *cdkCellDef="let row; let i = $index">
@@ -43,18 +44,18 @@ import {CdkSelection} from './selection';
             [cdkSelectionToggleValue]="row"
             [cdkSelectionToggleIndex]="i"
             (click)="toggler.toggle()"
-            [checked]="toggler.checked$ | async">
+            [checked]="toggler.checked | async">
       </td>
     </ng-container>
   `,
 })
 export class CdkSelectionColumn<T> implements OnInit, OnDestroy {
   /** Column name that should be used to reference this column. */
-  @Input()
-  get cdkSelectionColumnName(): string {
+  @Input('cdkSelectionColumnName')
+  get name(): string {
     return this._name;
   }
-  set cdkSelectionColumnName(name: string) {
+  set name(name: string) {
     this._name = name;
 
     this.syncColumnDefName();
@@ -71,8 +72,8 @@ export class CdkSelectionColumn<T> implements OnInit, OnDestroy {
   ) {}
 
   ngOnInit() {
-    if (!this.selection) {
-      throw new Error('CdkSelectionColumn: missing CdkSelection in the parent');
+    if (!this.selection && isDevMode()) {
+      throw Error('CdkSelectionColumn: missing CdkSelection in the parent');
     }
 
     this.syncColumnDefName();
@@ -82,7 +83,9 @@ export class CdkSelectionColumn<T> implements OnInit, OnDestroy {
       this._columnDef.headerCell = this._headerCell;
       this.table.addColumnDef(this._columnDef);
     } else {
-      throw new Error('CdkSelectionColumn: missing parent table');
+      if (isDevMode()) {
+        throw Error('CdkSelectionColumn: missing parent table');
+      }
     }
   }
 

src/cdk-experimental/selection/selection-set.ts

@@ -6,14 +6,18 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {TrackByFunction} from '@angular/core';
+import {isDevMode, TrackByFunction} from '@angular/core';
 import {Subject} from 'rxjs';
 
+/**
+ * Maintains a set of selected values. One or more values can be added to or removed from the
+ * selection.
+ */
 interface TrackBySelection<T> {
   isSelected(value: SelectableWithIndex<T>): boolean;
   select(...values: Array<SelectableWithIndex<T>>): void;
   deselect(...values: Array<SelectableWithIndex<T>>): void;
-  changed$: Subject<SelectionChange<T>>;
+  changed: Subject<SelectionChange<T>>;
 }
 
 /**
@@ -42,7 +46,7 @@ export interface SelectionChange<T> {
  */
 export class SelectionSet<T> implements TrackBySelection<T> {
   private selectionMap = new Map<T|ReturnType<TrackByFunction<T>>, SelectableWithIndex<T>>();
-  changed$ = new Subject<SelectionChange<T>>();
+  changed = new Subject<SelectionChange<T>>();
 
   constructor(private _multiple = false, private _trackByFn?: TrackByFunction<T>) {}
 
@@ -51,8 +55,8 @@ export class SelectionSet<T> implements TrackBySelection<T> {
   }
 
   select(...selects: Array<SelectableWithIndex<T>>) {
-    if (!this._multiple && selects.length > 1) {
-      throw new Error('SelectionSet: not multiple selection');
+    if (!this._multiple && selects.length > 1 && isDevMode()) {
+      throw Error('SelectionSet: not multiple selection');
     }
 
     const before = this._getCurrentSelection();
@@ -73,12 +77,12 @@ export class SelectionSet<T> implements TrackBySelection<T> {
 
     const after = this._getCurrentSelection();
 
-    this.changed$.next({before, after});
+    this.changed.next({before, after});
   }
 
   deselect(...selects: Array<SelectableWithIndex<T>>) {
-    if (!this._multiple && selects.length > 1) {
-      throw new Error('SelectionSet: not multiple selection');
+    if (!this._multiple && selects.length > 1 && isDevMode()) {
+      throw Error('SelectionSet: not multiple selection');
     }
 
     const before = this._getCurrentSelection();
@@ -94,7 +98,7 @@ export class SelectionSet<T> implements TrackBySelection<T> {
     }
 
     const after = this._getCurrentSelection();
-    this.changed$.next({before, after});
+    this.changed.next({before, after});
   }
 
   private _markSelected(key: T|ReturnType<TrackByFunction<T>>, toSelect: SelectableWithIndex<T>) {
@@ -110,11 +114,11 @@ export class SelectionSet<T> implements TrackBySelection<T> {
       return select.value;
     }
 
-    if (select.index == null) {
-      throw new Error('SelectionSet: index required when trackByFn is used.');
+    if (select.index == null && isDevMode()) {
+      throw Error('SelectionSet: index required when trackByFn is used.');
     }
 
-    return this._trackByFn(select.index, select.value);
+    return this._trackByFn(select.index!, select.value);
   }
 
   private _getCurrentSelection(): Array<SelectableWithIndex<T>> {