Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auto-init): Convert JS to TypeScript #4395

Merged
merged 13 commits into from
Feb 14, 2019

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Feb 9, 2019

Refs #4225

@codecov-io
Copy link

codecov-io commented Feb 9, 2019

Codecov Report

Merging #4395 into feat/typescript will increase coverage by 0.03%.
The diff coverage is 92.3%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           feat/typescript    #4395      +/-   ##
===================================================
+ Coverage             98.9%   98.93%   +0.03%     
===================================================
  Files                   95       95              
  Lines                 6109     6102       -7     
  Branches               802      800       -2     
===================================================
- Hits                  6042     6037       -5     
+ Misses                  66       64       -2     
  Partials                 1        1
Impacted Files Coverage Δ
packages/mdc-auto-init/index.ts 95.23% <92.3%> (ø)
packages/mdc-top-app-bar/standard/foundation.js
packages/mdc-top-app-bar/foundation.js
packages/mdc-top-app-bar/short/foundation.js
packages/mdc-top-app-bar/fixed/foundation.js
packages/mdc-top-app-bar/index.js
packages/mdc-top-app-bar/fixed/foundation.ts 100% <0%> (ø)
packages/mdc-top-app-bar/foundation.ts 100% <0%> (ø)
packages/mdc-top-app-bar/index.ts 100% <0%> (ø)
... and 2 more

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 f8ba48f...5d3f489. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

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

packages/mdc-auto-init/index.ts Outdated Show resolved Hide resolved
packages/mdc-auto-init/index.ts Outdated Show resolved Hide resolved
@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

packages/mdc-auto-init/index.ts Show resolved Hide resolved
packages/mdc-auto-init/types.ts Outdated Show resolved Hide resolved
packages/mdc-auto-init/index.ts Outdated Show resolved Hide resolved
packages/mdc-auto-init/index.ts Show resolved Hide resolved
@mdc-web-bot
Copy link
Collaborator

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

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment regarding an old TODO, otherwise LGTM

warn(`(mdc-auto-init) Component already initialized for ${node}. Skipping...`);
continue;
}

// TODO: Should we make an eslint rule for an attachTo() static method?
const component = Ctor.attachTo(node);
// See https://github.com/Microsoft/TypeScript/issues/14600 for discussion of static interface support in TS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the TODO above this obsolete? (Like, would our build actually fail without an attachTo given the typings above? Plus we're using tslint now to reuse the Google configuration.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately TypeScript won't catch that because it doesn't do static type checking.
See microsoft/TypeScript#14600.

attachTo() in the Component interface is only there to make TS happy when we try to call Constructor.attachTo() in mdcAutoInit(). It doesn't verify that static attachTo() exists on classes passed to register().

It does, however, verify that the required constructor signature exists (via new<F...>()) on classes passed to register().

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@acdvorak acdvorak merged commit 4675c95 into feat/typescript Feb 14, 2019
@acdvorak acdvorak deleted the feat/typescript--auto-init branch February 14, 2019 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants