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

chore: Remove Object.assign from TS files #4389

Merged
merged 10 commits into from
Feb 11, 2019

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Feb 9, 2019

Refs #4225

This PR improves consistency across TS files:

  • Replaces Object.assign() with {...spread} operator
  • Changes foundation constructor arg types to Partial<MDCFooAdapter>
  • Uses ponyfill.matches() from mdc-dom instead of getMatchesProperty() from mdc-ripple
    • getMatchesProperty() still exists to avoid breaking backward compatibility, but it is no longer used in TS
  • Standardizes naming: evt, evtType, and <K extends EventType>
  • Always import /index (e.g., @material/foo/index instead of @material/foo)

@codecov-io
Copy link

codecov-io commented Feb 9, 2019

Codecov Report

Merging #4389 into feat/typescript will decrease coverage by <.01%.
The diff coverage is 95.31%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           feat/typescript    #4389      +/-   ##
===================================================
- Coverage            98.65%   98.65%   -0.01%     
===================================================
  Files                   93       94       +1     
  Lines                 5947     5937      -10     
  Branches               795      795              
===================================================
- Hits                  5867     5857      -10     
  Misses                  79       79              
  Partials                 1        1
Impacted Files Coverage Δ
packages/mdc-grid-list/foundation.ts 100% <ø> (ø) ⬆️
packages/mdc-menu-surface/index.ts 100% <ø> (ø) ⬆️
packages/mdc-base/component.ts 96.42% <ø> (-0.13%) ⬇️
packages/mdc-slider/foundation.ts 100% <100%> (ø) ⬆️
packages/mdc-dialog/foundation.ts 100% <100%> (ø) ⬆️
packages/mdc-floating-label/foundation.ts 100% <100%> (ø) ⬆️
packages/mdc-checkbox/foundation.ts 96.96% <100%> (ø) ⬆️
packages/mdc-snackbar/foundation.ts 100% <100%> (ø) ⬆️
packages/mdc-ripple/foundation.ts 100% <100%> (ø) ⬆️
packages/mdc-menu-surface/foundation.ts 100% <100%> (ø) ⬆️
... and 21 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 e95ff8a...9e94771. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@kfranqueiro kfranqueiro mentioned this pull request Feb 9, 2019
45 tasks
@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@mdc-web-bot
Copy link
Collaborator

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

@kfranqueiro
Copy link
Contributor

Using ponyfill.matches everywhere will end up fixing #4340, which someone submitted #4372 for against master originally (I suggested waiting until the TS conversion).

@@ -44,8 +44,10 @@ class MDCGridListFoundation extends MDCFoundation<MDCGridListAdapter> {
private readonly resizeHandler_: EventListener;
private resizeFrame_ = 0;

constructor(adapter: MDCGridListAdapter) {
super(Object.assign(MDCGridListFoundation.defaultAdapter, adapter));
/* istanbul ignore next: optional argument is not a branch statement */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ignore next still necessary without a default assignment? I don't see it on the other constructors using an optional argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm honestly not sure why it's needed in some places but not others.

I've noticed that it's usually needed when a base foundation is subclassed by another foundation that doesn't have its own constructor (e.g., tab-scroller), but this case I have no idea.

My only guess is that it's a quirk in Istanbul.

packages/mdc-radio/index.ts Outdated Show resolved Hide resolved
packages/mdc-ripple/index.ts Outdated Show resolved Hide resolved
test/unit/mdc-ripple/mdc-ripple.test.js Outdated Show resolved Hide resolved
@@ -51,6 +51,7 @@ class MDCComponent<FoundationType extends MDCFoundation> {
this.initialSyncWithDOM();
}

/* istanbul ignore next: method param only exists for typing purposes; it does not need to be unit tested */
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this comment skipping? There's no default assignment and I don't see any conditionals in the output code... shouldn't this end up hit during our unit tests anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Welcome to Istanbul 😛 If you remove this comment, code coverage drops on certain components. Don't ask me to explain it, cuz I gave up trying to figure it out after 30 min of banging my head against a wall.

@mdc-web-bot
Copy link
Collaborator

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

@acdvorak acdvorak merged commit b859440 into feat/typescript Feb 11, 2019
@acdvorak acdvorak deleted the feat/typescript--object-assign branch February 11, 2019 20:39
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.

5 participants