feat(select): Convert JS to TypeScript #4386

acdvorak · 2019-02-09T04:22:59Z

packages/mdc-select/types.ts

codecov-io · 2019-02-09T04:33:37Z

Codecov Report

Merging #4386 into feat/typescript will increase coverage by <.01%.
The diff coverage is 98.79%.

@@                 Coverage Diff                 @@
##           feat/typescript    #4386      +/-   ##
===================================================
+ Coverage            98.68%   98.69%   +<.01%     
===================================================
  Files                   94       95       +1     
  Lines                 5987     6032      +45     
  Branches               805      797       -8     
===================================================
+ Hits                  5908     5953      +45     
  Misses                  78       78              
  Partials                 1        1

Impacted Files	Coverage Δ
packages/mdc-base/component.ts	`96.42% <ø> (ø)`	⬆️
packages/mdc-select/helper-text/foundation.ts	`100% <100%> (ø)`
packages/mdc-select/foundation.ts	`97.6% <100%> (ø)`
packages/mdc-select/icon/index.ts	`100% <100%> (ø)`
packages/mdc-select/helper-text/index.ts	`100% <100%> (ø)`
packages/mdc-list/index.ts	`98.51% <100%> (ø)`	⬆️
packages/mdc-select/icon/foundation.ts	`100% <100%> (ø)`
packages/mdc-select/index.ts	`98.39% <98.39%> (ø)`
packages/mdc-base/index.ts	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2fa9a5...a0cce8c. Read the comment docs.

mdc-web-bot · 2019-02-09T17:55:49Z

All 621 screenshot tests passed for commit bc8bf0d vs. feat/typescript! 💯🎉

…nts/material-components-web into feat/typescript--select

mdc-web-bot · 2019-02-09T21:20:45Z

All 621 screenshot tests passed for commit 7a7d960 vs. feat/typescript! 💯🎉

moog16 · 2019-02-11T18:55:58Z

packages/mdc-list/index.ts

@@ -33,8 +33,8 @@ class MDCList extends MDCComponent<MDCListFoundation> {
    this.foundation_.setVerticalOrientation(value);
  }

-  get listElements(): Element[] {
-    return [].slice.call(this.root_.querySelectorAll(strings.ENABLED_ITEMS_SELECTOR));
+  get listElements(): HTMLElement[] {


is it guaranteed that this will always be HTMLElement? I think so, but just wondering what you were thinking.

I think it was because we're calling .focus() on these items in a few places, and I assumed .mdc-list-item would always be an HTMLElement anyway, so I thought we could avoid type assertions by just declaring it an HTMLElement.

However, it looks like it's pretty easy to change this back to Element if you prefer (take a look at commit 91f5975).

I figured it's probably OK to return a more specific subtype, as long as our arguments are still Element for Closure users.

WDYT?

packages/mdc-menu/types.ts

packages/mdc-select/foundation.ts

packages/mdc-select/icon/foundation.ts

packages/mdc-select/package.json

…ent[]`

…e.json`

…cript--select

mdc-web-bot · 2019-02-11T23:44:51Z

All 624 screenshot tests passed for commit f6de964 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-12T00:02:08Z

All 624 screenshot tests passed for commit 4fa0e79 vs. feat/typescript! 💯🎉

…cript--select

mdc-web-bot · 2019-02-12T05:37:19Z

All 624 screenshot tests passed for commit dc34eb6 vs. feat/typescript! 💯🎉

packages/mdc-base/component.ts

kfranqueiro · 2019-02-12T19:20:26Z

packages/mdc-list/index.ts

@@ -135,7 +135,7 @@ class MDCList extends MDCComponent<MDCListFoundation> {
        }
      },
      focusItemAtIndex: (index) => {
-        const element = this.listElements[index] as HTMLElement;
+        const element = this.listElements[index] as HTMLElement | null;


Wouldn't this technically be undefined, not null, if it didn't exist? What does this fix?

You are technically correct! 😀 Fixed.

There's a subtle but important difference between a type assertion with and without | null (or | undefined):

// This tells the TypeScript compiler that `element` will // NEVER be null or undefined, so it won't yell at you // if you try to access a property on `element`, which is dangerous. const element = this.listElements[index] as HTMLElement; // This tells tsc that `element` might not have a value, so it should // yell at you if you try to access a property without a truthy check. const element = this.listElements[index] as HTMLElement | undefined;

In this specific case, it doesn't matter quite as much because we already have a truthy check below (if (element) { ... }), but if we ever refactor this code, we could easily forget that element might be undefined.

Personally, I'd rather be semantically accurate by declaring that the type might be undefined.

kfranqueiro · 2019-02-12T19:34:16Z

packages/mdc-menu/types.ts

@@ -24,6 +24,9 @@
 import {MDCList} from '@material/list/index';
 import {MDCMenuSurface} from '@material/menu-surface/index';

+export type MenuItemEvent = CustomEvent<MenuItemEventDetail>;
+export type DefaultMenuItemEvent = CustomEvent<DefaultMenuItemEventDetail>;


I guess I overlooked this in the original Menu PR but I found the Default here confusing. Unsure whether #4407 could leave room for resolving this.

I had a hard time figuring out what to call this type, so I'm definitely open to suggestions.

MenuItemEvent is only used by foundation and adapter

DefaultMenuItemEvent is only used by the component, and has an additional property on its detail object

What would you prefer to call it?

packages/mdc-select/helper-text/index.ts

packages/mdc-select/icon/README.md

packages/mdc-select/index.ts

kfranqueiro · 2019-02-12T20:17:15Z

packages/mdc-select/types.ts

+  index: number;
+}
+
+// TODO(acdvorak): Every component should export its own factory and event types.


Define "every"? This is pretty particular to text field and select sub-components.

A number of components need to instantiate ripples, icons, etc. I figured we might as well just make it a standard pattern to always export a factory type for every component so that we don't end up with duplicate factory definitions in lots of different components.

moog16 · 2019-02-12T22:22:12Z

packages/mdc-select/index.ts

+    element.addEventListener('blur', this.handleBlur_);
+
+    MOUSEDOWN_TOUCHSTART_EVENTS.forEach((evtType) => {
+      element.addEventListener(evtType, this.handleClick_ as EventListener);


since element can potentially be null, I think we need a guard here.

The component is only valid if nativeInput or selectedText is present, and the code above declares that selectedText will be defined if nativeInput isn't (there's also a comment above it).

element should never be null:

// One of these elements must be non-null. const element = this.nativeControl_ || this.selectedText_!;

Should we then throw an error if both are missing? It seems like a helpful user error to know why the component wouldn't be working. I guess it is obvious if you don't have a select/list, just throwing out ideas.

Yeah, that sounds like a good idea. I'll do that and add a unit test.

packages/mdc-select/index.ts

moog16 · 2019-02-12T22:26:41Z

packages/mdc-select/index.ts

+   */
+  private getNormalizedXCoordinate_(evt: MouseEvent): number {
+    const targetClientRect = (evt.target as Element).getBoundingClientRect();
+    const xCoordinate = evt.clientX; // TODO(acdvorak): How should this be typed?


isn't this inferred to be a number already?

The problem is that this method actually takes a MouseEvent | TouchEvent, and clientX isn't defined on TouchEvent. I'm actually not sure how this code works at all right now.

type PointerEventType = 'mousedown' | 'touchstart'; const POINTER_EVENTS: PointerEventType[] = ['mousedown', 'touchstart']; class MDCSelect { private handleClick_!: SpecificEventListener<PointerEventType>; // assigned in initialize() initialSyncWithDOM() { this.handleClick_ = (evt) => { if (this.selectedText_) { this.selectedText_.focus(); } this.foundation_.handleClick(this.getNormalizedXCoordinate_(evt)); }; POINTER_EVENTS.forEach((evtType) => { element.addEventListener(evtType, this.handleClick_ as EventListener); }); } private getNormalizedXCoordinate_(evt: MouseEvent | TouchEvent): number { const targetClientRect = (evt.target as Element).getBoundingClientRect(); const xCoordinate = evt.clientX; // TODO(acdvorak): How should this be typed? return xCoordinate - targetClientRect.left; } }

OH! Wait I think there is a bug for this...I don't think it works.

Having a hard time finding it, but I do believe this was an issue in either React or Web or both.

material-components/material-components-web-react#275 (comment)

Didn't we fix this somewhere to dig out touches[0], which does have clientX?

The above issue I linked to is text field. So I think we also need to fix for select.

Good call! A type guard should do the trick:

class MDCSelect { private getNormalizedXCoordinate_(evt: MouseEvent | TouchEvent): number { const targetClientRect = (evt.target as Element).getBoundingClientRect(); const xCoordinate = this.isTouchEvent_(evt) ? evt.touches[0].clientX : evt.clientX; return xCoordinate - targetClientRect.left; } private isTouchEvent_(evt: MouseEvent | TouchEvent): evt is TouchEvent { return Boolean((evt as TouchEvent).touches); } }

mdc-web-bot · 2019-02-12T22:49:10Z

All 624 screenshot tests passed for commit ebbbbb9 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-12T23:03:37Z

All 624 screenshot tests passed for commit a236050 vs. feat/typescript! 💯🎉

moog16 · 2019-02-12T23:39:03Z

packages/mdc-select/index.ts

@@ -336,14 +239,111 @@ class MDCSelect extends MDCComponent<MDCSelectFoundation> implements RippleCapab
    super.destroy();
  }

+  get value(): string {


wow I wish Github would just move the initialize method lol

mdc-web-bot · 2019-02-12T23:39:46Z

All 624 screenshot tests passed for commit 1e8b7e2 vs. feat/typescript! 💯🎉

moog16 · 2019-02-12T23:42:29Z

packages/mdc-select/index.ts

+  private nativeControl_!: HTMLSelectElement | null; // assigned in initialize()
+  private selectedText_!: HTMLElement | null; // assigned in initialize()
+
+  private targetElement_!: HTMLElement; // assigned in initialize()


good clean up idea!

mdc-web-bot · 2019-02-12T23:49:29Z

All 624 screenshot tests passed for commit a0cce8c vs. feat/typescript! 💯🎉

moog16 · 2019-02-12T23:51:18Z

packages/mdc-select/index.ts

    return xCoordinate - targetClientRect.left;
  }

+  private isTouchEvent_(evt: MouseEvent | TouchEvent): evt is TouchEvent {


what if we changed this to just getXCoordinateFromEvent? Then we just encapsulate this this.isTouchEvent_(evt) ? evt.touches[0].clientX : evt.clientX; logic...cause we still end up having to cast it anyways inside this method.

As discussed offline, it would need to look like this:

private getNormalizedXCoordinate_(evt: MouseEvent | TouchEvent): number { const targetClientRect = (evt.target as Element).getBoundingClientRect(); return this.getClientXFromEvent_(evt) - targetClientRect.left; } private getClientXFromEvent_(evt: MouseEvent | TouchEvent): number { const touchEvt = evt as TouchEvent; const mouseEvt = evt as MouseEvent; if (touchEvt.touches) { return touchEvt.touches[0].clientX; } return mouseEvt.clientX; }

Matt and I prefer the isTouchEvent_() style, so we're going to leave it as-is.

moog16

Looks good!

Addressed Ken's comments; approved by Matt

feat(select): Convert JS to TypeScript

9313e66

Refs #4225

acdvorak added the do-not-merge label Feb 9, 2019

acdvorak assigned kfranqueiro and moog16 Feb 9, 2019

acdvorak requested review from kfranqueiro and moog16 February 9, 2019 04:22

googlebot added the cla: yes label Feb 9, 2019

kfranqueiro mentioned this pull request Feb 9, 2019

Convert packages to TypeScript #4225

Closed

45 tasks

acdvorak commented Feb 9, 2019

View reviewed changes

packages/mdc-select/types.ts Outdated Show resolved Hide resolved

WIP: Add comment

135e09d

WIP: Fix compiler errors

bc8bf0d

acdvorak added 2 commits February 9, 2019 13:04

WIP: Make Istanbul happy

fb0b996

Merge branch 'feat/typescript--select' of github.com:material-compone…

7a7d960

…nts/material-components-web into feat/typescript--select

moog16 suggested changes Feb 11, 2019

View reviewed changes

acdvorak added 6 commits February 11, 2019 13:40

WIP: Change MDCList.listElements from HTMLElement[] back to `Elem…

91f5975

…ent[]`

WIP: extends CustomEvent<MenuItemEventDetail>

b50a47a

WIP: Remove Closure annotations

01edf0f

WIP: Initialize savedTabIndex_ inline

a286663

WIP: Remove unnecessary @material/textfield dependency from `packag…

5facf07

…e.json`

Merge remote-tracking branch 'origin/feat/typescript' into feat/types…

7b625f6

…cript--select

acdvorak removed the do-not-merge label Feb 11, 2019

Merge remote-tracking branch 'origin/feat/typescript' into feat/types…

f6de964

…cript--select

WIP: Update filename to .ts in js-bundle-factory.js

4fa0e79

Merge remote-tracking branch 'origin/feat/typescript' into feat/types…

dc34eb6

…cript--select

kfranqueiro previously requested changes Feb 12, 2019

View reviewed changes

WIP: Revert argument rename in MDCComponent.emit()

de38dd6

moog16 suggested changes Feb 12, 2019

View reviewed changes

acdvorak added 3 commits February 12, 2019 14:29

WIP: Address review comments

01b22be

WIP: Re-indent

ebbbbb9

WIP: Address review comments

a236050

acdvorak added 2 commits February 12, 2019 15:23

WIP: Throw an error if required subelements are missing

1e8b7e2

WIP: Fix TouchEvent clientX bug

a0cce8c

moog16 reviewed Feb 12, 2019

View reviewed changes

moog16 approved these changes Feb 13, 2019

View reviewed changes

acdvorak merged commit 5052ada into feat/typescript Feb 13, 2019

acdvorak deleted the feat/typescript--select branch February 13, 2019 00:17

feat(select): Convert JS to TypeScript #4386

feat(select): Convert JS to TypeScript #4386

Conversation

acdvorak commented Feb 9, 2019

codecov-io commented Feb 9, 2019 • edited Loading

Codecov Report

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 12, 2019

mdc-web-bot commented Feb 12, 2019

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

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

mdc-web-bot commented Feb 12, 2019

mdc-web-bot commented Feb 12, 2019

Choose a reason for hiding this comment

mdc-web-bot commented Feb 12, 2019

Choose a reason for hiding this comment

mdc-web-bot commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 9, 2019 •

edited

Loading