-
Notifications
You must be signed in to change notification settings - Fork 208
Conversation
typings/preact/helpers.d.ts
Outdated
type Diff< | ||
T extends string | number | symbol, | ||
U extends string | number | symbol | ||
> = ({[P in T]: P} & {[P in U]: never} & {[x: string]: never})[T] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, after fix this, Glamorous typing will not work with TS <= 2.8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, have you tested that? I'm guessing the in T
and in U
don't work? this is why flow's approach is better (versioning to lib and flow instead of just lib). Any way, it seems like we should prefer supporting the most recent version of TS, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronjensen As you said, supporting recent TS version is better. However, if there's a way to support both old and new versions, that's the best, isn't it?
I mean, keyof any
will return string | number | symbol
for TS@2.9, and string
for TS@<=2.8, so how about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we want to support TS@2.9 only, we have Extract
from TS@2.8, so we don't need these helpers.
@Ailrun I used |
@aaronjensen So for you, does |
@Ailrun no, to be honest, I don't know what the full impacts would be of that change so I didn't want to risk it and given that there won't be much other reason to upgrade glamorous it doesn't seem like a big deal to drop support for <2.8 |
Can we get the build working before we merge this please? :) |
We could yeah, but it has nothing to do w/ this PR, so should it be a separate one? This repo doesn't appear to use a package-lock or a yarn.lock so as new eslint rules get added failures crop up with 0 changes. Current failures:
|
I actually get even more lint failures locally 😖 |
Codecov Report
@@ Coverage Diff @@
## master #425 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 10 10
Lines 177 177
Branches 50 50
=====================================
Hits 177 177 Continue to review full report at Codecov.
|
Tests are passing now with the deprecated lints being ignored. Maybe a follow-up PR could deal w/ it properly (which would probably require bumping the minimum version of react required) |
@@ -71,7 +71,7 @@ | |||
"tslint": "^5.8.0", | |||
"tslint-microsoft-contrib": "^5.0.0", | |||
"tsutils": "^2.15.0", | |||
"typescript": "^2.6.2" | |||
"typescript": "^2.9.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not this work with TS@2.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snapshot tests won't. This is just a dev dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, snapshot... OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.