TS 3.2 updates by sandersn · Pull Request #30700 · DefinitelyTyped/DefinitelyTyped

sandersn · 2018-11-20T19:36:37Z

Update bluebird to account for differences in Typescript 3.2 assignability rules by casting away the call signature of ErrorConstructor.
Update match-media-mock to with the correct media query list listener API.
Update redux-action to redux 4.0, so that usage matches the tests.
Remove unneeded dependency on ReduxThunk from redux-action.

Note that all changes are to tests, except for the update to redux 4.0 in redux-action's package.json.

This helps type inference work correctly.

typescript-bot · 2018-11-20T20:22:27Z

@sandersn Thank you for submitting this PR!

🔔 @lhecker @nickiannone @asvetliakov @newraina - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

typescript-bot · 2018-11-20T20:22:28Z

@sandersn The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

lhecker · 2018-11-20T20:29:19Z

types/bluebird/bluebird-tests.ts


 // $ExpectType Bluebird<void | Foo>
-fooProm.catch(Error, (reason: any) => {
+fooProm.catch(Error as { new(message?: string): Error }, (reason: any) => {


I don't quite understand the changes to this file. Can you explain to me why they're necessary?
(I haven't been following the latest changes in TS 3.2. 🤐)

I'm asking because because these particular tests assert the proper functionality of one of Bluebird's features.
As such it's important that the Bluebird typings work even if you keep this test file as it is.

Would it be possible to change the actual typings instead?

Error actually appears to match two of the possible filter types: class ErrorClass and function (anyOtherError), but the second match is incorrect. The typescript representation is

type CatchFilter<E> = (new (...args: any[]) => E) | ((error: E) => boolean) interface ErrorConstructor { new(message?: string): Error; (message?: string): Error; }

Inferring from the construct signature gives E=Error, and inferring from the call signature gives E=string | undefined. I'm not sure why pre-3.2 Typescript used the first inference instead of the second.

The change happened in microsoft/TypeScript#27028, which changed the way co- and contra-variant inferences work so that contravariant inferences (callback parameters like (error: E) => boolean) are preferred over covariant inferences (return positions like new (...args:any[]) => E).

I think this is the right change for most cases, so I'll see if I can find a workaround for this case.

Ah I see... That's of course very unfortunate since we were relying on the CatchFilter preferring covariants / the constructor signature. 😟
It'd be nice if we could come up with a solution that properly resolves this. 🤔
I'll try to come up with something as well, although I'm not as versed in typescript as I used to.

Well, I can manually promote the constructor inferences by breaking catch into more overloads. I'm trying that now to make sure it works and see how ugly it is.

If it works I'm absolutely happy with it. 🙂

Meanwhile I tried:

type CatchFilter<E> = E extends InstanceType<infer R> ? R : (((error: E) => boolean) | (object & E));

But it doesn't work: It still resolves to / picks the latter half and thus fails the type check.
I thought this ternary-like operator would actually behave like a classic ternary operator and be evaluated eagerly. But instead it seems that it's resolved just like the regular union operator?

1. Use 3.1 and 3.2-compatible fix for match-media-mock. Specify a this-parameter. 2. redux-action requires TS 2.3 so that type parameter defaults and mapped types from redux 4.0 work.

typescript-bot · 2018-11-20T21:22:35Z

@sandersn One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

typescript-bot · 2018-11-20T21:22:36Z

🔔 @lhecker - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

Notes: 1. I only duplicated overloads to get 2 * n instead of 2 * n overloads needed to support every combination of constructor function/other. 2. I deleted the duplicate R vs U | R overloads for `catch` since they were equivalent; I don't think the R overloads were ever used. 3. The original overload is now Constructor<E> | CatchFilter<E>; I haven't figured out why yet, but normal classes don't use the new Constructor<E>-only overload. Only constructor functions do.

sandersn · 2018-11-21T00:05:20Z

Well, I created more overloads. There are some caveats that come with this commit:

I only duplicated overloads to get 2 * n instead of 2 * n overloads needed to support every combination of constructor function/other.
I deleted the duplicate R vs U | R overloads for catch since they were equivalent; I don't think the R overloads were ever used.
The original overload is now Constructor | CatchFilter; I haven't figured out why yet, but normal classes don't use the new Constructor-only overload. Only constructor functions do.

@weswigham Any clever ideas here?

lhecker · 2018-11-21T00:13:23Z

I believe the Bluebird<R> overloads were specifically created to aid type resolution.
Back then type unions like R | R didn't collapse into R, which is why Bluebird<R | U> created quite ugly looking type informations in the various editors (e.g. Bluebird<Foo | Foo> instead of Bluebird<Foo>).
AFAICS this has changed in the past year though didn't it?

Anyways... Thanks for the changes. 🙂

lhecker

I'm approving this already in order to not block you any further until you've resolved the remaining CI issues, as I'm probably preoccupied in the next 2 days or so. 🙂

sandersn · 2018-11-21T00:19:50Z

Thanks; it's just the linter warning me that overlapping overloads is a bad idea. I'll need to silence it if I decide to take this approach.

Tagging @DanielRosenwasser for clever workaround ideas too.

typescript-bot · 2018-11-21T00:22:42Z

@sandersn The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

types/match-media-mock/match-media-mock-tests.ts

types/db-migrate-base/db-migrate-base-tests.ts

types/project-oxford/project-oxford-tests.ts

typescript-bot · 2018-11-27T00:27:03Z

@sandersn I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

typescript-bot · 2018-11-28T00:28:38Z

@sandersn To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

typescript-bot · 2018-12-13T19:42:32Z

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

sandersn added 5 commits November 20, 2018 09:09

Update match-media-mock tests

a185047

Update redux-action with redux 4.0 API

9e68f23

Downcast Error types to ignore call signature

fd052a6

This helps type inference work correctly.

Merge branch 'master' into ts-3.2-dom-updates

db9dd24

Fix unrelated lints

f1d0311

typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Nov 20, 2018

typescript-bot added the The Travis CI build failed label Nov 20, 2018

lhecker suggested changes Nov 20, 2018

View reviewed changes

Fix CI failure

dc6cea2

1. Use 3.1 and 3.2-compatible fix for match-media-mock. Specify a this-parameter. 2. redux-action requires TS 2.3 so that type parameter defaults and mapped types from redux 4.0 work.

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed The Travis CI build failed labels Nov 20, 2018

lhecker approved these changes Nov 21, 2018

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. The Travis CI build failed labels Nov 21, 2018

Jessidhia reviewed Nov 21, 2018

View reviewed changes

types/match-media-mock/match-media-mock-tests.ts Show resolved Hide resolved

types/db-migrate-base/db-migrate-base-tests.ts Show resolved Hide resolved

types/project-oxford/project-oxford-tests.ts Show resolved Hide resolved

types/project-oxford/project-oxford-tests.ts Show resolved Hide resolved

newraina approved these changes Nov 24, 2018

View reviewed changes

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Nov 28, 2018

typescript-bot closed this Nov 28, 2018

Merge branch 'master' into ts-3.2-dom-updates

97f42ab

sandersn reopened this Dec 13, 2018

Fix lint in bluebird

8fe4901

typescript-bot added Merge:Express and removed The Travis CI build failed Abandoned This PR had no activity for a long time, and is considered abandoned labels Dec 13, 2018

sandersn merged commit 285f31b into master Dec 13, 2018

sjmarshy mentioned this pull request Dec 15, 2018

Update dependency @types/ramda to version 0.25.45 sjmarshy/gavel#13

Open

elibarzilay deleted the ts-3.2-dom-updates branch November 28, 2020 23:58

Conversation

sandersn commented Nov 20, 2018

Uh oh!

typescript-bot commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Nov 20, 2018

Uh oh!

lhecker Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

sandersn Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

sandersn Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

lhecker Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Nov 20, 2018

Uh oh!

typescript-bot commented Nov 20, 2018

Uh oh!

sandersn commented Nov 21, 2018

Uh oh!

lhecker commented Nov 21, 2018

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn commented Nov 21, 2018

Uh oh!

typescript-bot commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Nov 27, 2018

Uh oh!

typescript-bot commented Nov 28, 2018

Uh oh!

typescript-bot commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

typescript-bot commented Nov 20, 2018 •

edited

Loading

sandersn Nov 20, 2018 •

edited

Loading

lhecker Nov 20, 2018 •

edited

Loading

typescript-bot commented Nov 21, 2018 •

edited

Loading