feat: nullability support #5094

crisbeto · 2017-06-12T20:00:59Z

Adds compatibility with strictNullChecks to the library, tests, build and various test apps.

devversion · 2017-06-12T20:20:42Z

src/lib/slide-toggle/slide-toggle.ts

@@ -318,7 +318,7 @@ class SlideToggleRenderer {

  /** Resets the current drag and returns the new checked value. */
  stopThumbDrag(): boolean {
-    if (!this.dragging) { return; }
+    if (!this.dragging) { return this.dragging; }


Just a note for your wip: This is can be always return false

jelbourn

Looks good, just a handful of minor comments

jelbourn · 2017-06-13T16:38:33Z

src/lib/button-toggle/button-toggle.ts

@@ -65,13 +65,13 @@ export class MdButtonToggleGroup implements AfterViewInit, ControlValueAccessor
  private _name: string = `md-button-toggle-group-${_uniqueIdCounter++}`;

  /** Disables all toggles in the group. */
-  private _disabled: boolean = null;
+  private _disabled: boolean | null = null;


Why not default this to false?
(same for other boolean properties)

EDIT: Just saw your comment below. I'll refactor the bindings to disabled || null.

It's because the disabled attribute needs to be removed from the DOM completely when enabled. We could also switch the bindings to be something like disabled || null.

jelbourn · 2017-06-13T16:49:58Z

src/lib/core/datetime/native-date-adapter.ts

-    if (month < 0 || month > 11 || date < 1) {
-      return null;
+    if (month < 0 || month > 11) {
+      throw new Error(`Invalid month index "${month}". Month index has to be between 0 and 11.`);


Can omit the new for Error (woo, 4 bytes)

jelbourn · 2017-06-13T16:50:39Z

src/lib/core/datetime/native-date-adapter.ts

+    }
+
+    if (date < 1) {
+      throw new Error(`Invalid date "${date}". Date has to be larger than 0.`);


"larger than" > "greater than"

jelbourn · 2017-06-13T16:58:22Z

src/lib/core/overlay/position/connected-position-strategy.ts

@@ -94,7 +94,7 @@ export class ConnectedPositionStrategy implements PositionStrategy {
   * @param element Element to which to apply the CSS styles.
   * @returns Resolves when the styles have been applied.
   */
-  apply(element: HTMLElement): Promise<void> {
+  apply(element: HTMLElement): Promise<null> {


Can we work around this by having a custom type declaration to overload resolve such that resolve(): Promise<void>?

Promise<null> => (╯°□°）╯︵ ┻━┻

(alternatively, apply is and always has been synchronous, so we could just take the promise away)

Not sure what that would look like, null | undefined? Otherwise I'm in favor of removing the promise completely, although I could see it being useful if we decide to introduce animations into the overlays.

jelbourn · 2017-06-13T17:08:58Z

src/lib/core/portal/portal.ts

-      viewContainerRef: ViewContainerRef = null,
-      injector: Injector = null) {
+      viewContainerRef?: ViewContainerRef,
+      injector?: Injector) {


Could make these | null just to be more lenient (anywhere with multiple optional params where you might want to pass a latter one but an earlier one)

I went with optionals here, because they were marked as [Optional] a little bit up in the doc strings for the properties.

Sorry, I accidentally omitted the word "also". Meant that we could do
viewContainerRef?: ViewContainerRef | null, so that either undefined or null can be passed in cases when you don't care for that argument.

Makes sense. Added.

jelbourn · 2017-06-13T17:11:08Z

src/lib/core/ripple/ripple.spec.ts

  let viewportRuler: ViewportRuler;

+  /** Extracts the numeric value of a pixel size string like '123px'.  */
+  const pxStringToFloat = (s: string | null) => s ? parseFloat(s.replace('px', '')) : 0;


I know it was already like this, but doesn't parseFloat already ignore trailing non-numeric characters already?

It does. This can be changed to parseFloat(...) || 0.

jelbourn · 2017-06-13T17:16:59Z

src/lib/dialog/dialog.spec.ts

-
-      expect(() => dialogRef = dialog.open(DialogWithInjectedData)).not.toThrow();
-      expect(dialogRef.componentInstance.data).toBeNull();
+      // TODO: double check that this does what i expect


Ops, forgot to follow up on this one. I wanted to check that the expect inside the expect.not.toThrow won't cause any issues. I'll check it and refactor or remove the todo.

jelbourn · 2017-06-13T17:19:34Z

src/lib/icon/icon-registry.ts

+ * @docs-private
+ */
+export function getMdIconFailedToSanitizeError(url: SafeResourceUrl): Error {
+  return new Error(`MdIconRegistry has failed to sanitize the following URL: ${url}.`);


A more accurate error message would be something like "The url provided to MdIconRegistry was not trusted as a resource URL via Angular's DomSanitizer"

jelbourn · 2017-06-13T17:23:51Z

src/lib/menu/menu-item.ts

@@ -44,7 +44,7 @@ export class MdMenuItem implements Focusable {
  }

  /** Used to set the HTML `disabled` attribute. Necessary for links to be disabled properly. */
-  _getDisabledAttr(): boolean {
+  _getDisabledAttr(): true | null {


I'm a fan of doing disabled || null in the host binding

This means that the attribute is correctly removed in the DOM but programmatic APIs return the expected true/false

crisbeto · 2017-06-13T18:59:43Z

Rebased and addressed the feedback @jelbourn.

kara · 2017-06-16T22:38:37Z

@crisbeto This one needs rebase too, when you get a chance.

jelbourn · 2017-06-20T00:05:48Z

I rebased it (was trivial)

Adds compatibility with strictNullChecks to the library, tests, build and various test apps. Fixes angular#3486.

angular-automatic-lock-bot · 2019-09-06T14:02:05Z

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.}

crisbeto self-assigned this Jun 12, 2017

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 12, 2017

crisbeto mentioned this pull request Jun 12, 2017

feat: support strictNullChecks #3486

Closed

crisbeto force-pushed the strict-null-checks-redux branch from d1edd16 to 9d4c411 Compare June 12, 2017 20:16

devversion reviewed Jun 12, 2017

View reviewed changes

crisbeto force-pushed the strict-null-checks-redux branch 3 times, most recently from 7614fb6 to 62bd6f5 Compare June 12, 2017 20:59

crisbeto changed the title ~~wip: feat: nullability support~~ feat: nullability support Jun 12, 2017

crisbeto assigned jelbourn and unassigned crisbeto Jun 12, 2017

crisbeto added the pr: needs review label Jun 12, 2017

jelbourn reviewed Jun 13, 2017

View reviewed changes

crisbeto force-pushed the strict-null-checks-redux branch 3 times, most recently from c1c7dcd to 5e9b13e Compare June 13, 2017 18:59

crisbeto force-pushed the strict-null-checks-redux branch from 5e9b13e to 5ebfed6 Compare June 13, 2017 20:47

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 13, 2017

crisbeto force-pushed the strict-null-checks-redux branch 2 times, most recently from 08da2d8 to ced3171 Compare June 13, 2017 21:39

kara added presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged pr: needs rebase labels Jun 13, 2017

crisbeto force-pushed the strict-null-checks-redux branch from ced3171 to 02e42fb Compare June 14, 2017 06:26

crisbeto removed the pr: needs rebase label Jun 14, 2017

crisbeto force-pushed the strict-null-checks-redux branch from 02e42fb to d6f8f5e Compare June 16, 2017 16:46

kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jun 16, 2017

kara assigned crisbeto and unassigned jelbourn Jun 16, 2017

crisbeto force-pushed the strict-null-checks-redux branch from d6f8f5e to e6e96c1 Compare June 17, 2017 12:37

crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jun 17, 2017

jelbourn force-pushed the strict-null-checks-redux branch from e6e96c1 to 6b15671 Compare June 20, 2017 00:05

crisbeto force-pushed the strict-null-checks-redux branch from 6b15671 to 48e822b Compare June 20, 2017 17:29

crisbeto and others added 2 commits June 20, 2017 15:46

feat: nullability support

0c3b0df

Adds compatibility with strictNullChecks to the library, tests, build and various test apps. Fixes angular#3486.

Fix dialog tests that were added after initial strict-null pass

890052c

jelbourn force-pushed the strict-null-checks-redux branch from 48e822b to 890052c Compare June 20, 2017 22:56

jelbourn removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jun 21, 2017

jelbourn merged commit 2bf7024 into angular:master Jun 21, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: nullability support #5094

feat: nullability support #5094

crisbeto commented Jun 12, 2017

devversion Jun 12, 2017 •

edited

jelbourn left a comment

jelbourn Jun 13, 2017

crisbeto Jun 13, 2017 •

edited

jelbourn Jun 13, 2017

jelbourn Jun 13, 2017

jelbourn Jun 13, 2017

crisbeto Jun 13, 2017

jelbourn Jun 13, 2017

crisbeto Jun 13, 2017

jelbourn Jun 13, 2017

crisbeto Jun 13, 2017

jelbourn Jun 13, 2017

crisbeto Jun 13, 2017

jelbourn Jun 13, 2017

crisbeto Jun 13, 2017

jelbourn Jun 13, 2017

jelbourn Jun 13, 2017

crisbeto commented Jun 13, 2017

kara commented Jun 16, 2017

jelbourn commented Jun 20, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

feat: nullability support #5094

feat: nullability support #5094

Conversation

crisbeto commented Jun 12, 2017

devversion Jun 12, 2017 • edited

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jun 13, 2017

kara commented Jun 16, 2017

jelbourn commented Jun 20, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

devversion Jun 12, 2017 •

edited

crisbeto Jun 13, 2017 •

edited