feat(list): ts conversion #4334
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4334 +/- ##
==================================================
+ Coverage 98.39% 98.4% +<.01%
==================================================
Files 93 93
Lines 5803 5819 +16
Branches 784 783 -1
==================================================
+ Hits 5710 5726 +16
Misses 92 92
Partials 1 1
Continue to review full report at Codecov.
|
|
All 621 screenshot tests passed for commit 6a66c5d vs. |
|
All 621 screenshot tests passed for commit 7425047 vs. |
…f (causes problems with type checking)
|
All 621 screenshot tests passed for commit 6d12928 vs. |
packages/mdc-base/component.ts
Outdated
| @@ -98,7 +98,7 @@ class MDCComponent<FoundationType extends MDCFoundation> { | |||
| * Fires a cross-browser-compatible custom event from the component root of the given type, | |||
| * with the given data. | |||
| */ | |||
| emit(evtType: string, evtData: object, shouldBubble = false) { | |||
| emit(evtType: string, evtData: number | object, shouldBubble = false) { | |||
There was a problem hiding this comment.
I believe you can use unknown instead of number | object here. It will allow you to accept number, object, string, etc. but you won't be able to do things to it (e.g. access properties), which makes it safe, unlike any. (read more here)
There was a problem hiding this comment.
Thanks for the suggestion!
Ken and I thought it might be better to make this method generic, so that the caller can express the exact type of evtData. WDYT?
There was a problem hiding this comment.
Yeah you could do that too. I'm not sure specifying the type really does anything for you though, it doesn't get saved on the instance or used in the return value, it just gets handed off to browser APIs
There was a problem hiding this comment.
Fair point 😄
However, emit<T extends object>() has the advantage that it prevents primitive types like number from being passed to evtData, which we were previously doing in MDCList (see #4356).
To avoid backward compatibility issues in the future, we should always pass some kind of object so that we can easily add more properties.
OOC, what's the difference between
emit<T extends object>() and emit<T = object>()?
Edit: emit<T = object>() is incorrect. It sets the default type to object, but still allows primitive types like number, whereas we want to disallow primitives.
So emit<T extends object>() is indeed correct.
There was a problem hiding this comment.
If you want to disallow primitives, you can just do eventData: object. It looked from the existing code like you wanted to be able to pass objects or numbers (or I assumed anything else which is why I suggested unknown)
There was a problem hiding this comment.
True, but making it generic will also help protect us against accidentally passing the wrong data. E.g.:
interface Foo {
foo: number;
}
interface Bar {
bar: string;
}
this.emit<Foo>('whatever', {bar: '...'}); // Error
this.emit('whatever', {bar: '...'}); // "Valid", but incorrect
|
All 621 screenshot tests passed for commit ca9ecb1 vs. |
|
All 621 screenshot tests passed for commit 24c4533 vs. |
|
All 621 screenshot tests passed for commit b6231a9 vs. |
|
That's true...when I first saw that we were just passing a number, I was
wondering if that worked correctly (I'm assuming it does).
`emit<T = object>` defaults `T` to `object`. `<T extends object` ensures
that `T` is an `object`. You could also write `<T extends object = object>`.
…On Tue, Feb 5, 2019 at 5:47 PM Andrew C. Dvorak ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/mdc-base/component.ts
<#4334 (comment)>
:
> @@ -98,7 +98,7 @@ class MDCComponent<FoundationType extends MDCFoundation> {
* Fires a cross-browser-compatible custom event from the component root of the given type,
* with the given data.
*/
- emit(evtType: string, evtData: object, shouldBubble = false) {
+ emit(evtType: string, evtData: number | object, shouldBubble = false) {
Fair point 😄
However, emit<T extends object>() has the advantage that it prevents
primitive types like number from being passed to evtData, which we were
previously doing in MDCList (see #4356
<#4356>
).
To avoid backward compatibility issues in the future, we should always
pass some kind of object so that we can easily add more properties.
OOC, what's the difference between emit<T extends object>() and emit<T =
object>()?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4334 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjZIVxV1eOxzEZ1tyHX3XySZuEJZO5tks5vKjQggaJpZM4adokI>
.
|
|
All 621 screenshot tests passed for commit 7b4c0d4 vs. |
|
All 621 screenshot tests passed for commit 7338e9f vs. |
|
All 621 screenshot tests passed for commit 4e4c73d vs. |
|
All 621 screenshot tests passed for commit 11da4b2 vs. |
|
All 621 screenshot tests passed for commit 696fa69 vs. |
related #4225