Node: specify UnhandledRejectionListener reason param type
#24417
Conversation
|
@OliverJAsh Thank you for submitting this PR! 🔔 @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @eps1lon @Hannes-Magnusson-CK @jkomyno @ajafff @hoo29 @n-e - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
types/node/index.d.ts
Outdated
| @@ -510,7 +510,7 @@ declare namespace NodeJS { | |||
| type ExitListener = (code: number) => void; | |||
| type RejectionHandledListener = (promise: Promise<any>) => void; | |||
| type UncaughtExceptionListener = (error: Error) => void; | |||
| type UnhandledRejectionListener = (reason: any, promise: Promise<any>) => void; | |||
| type UnhandledRejectionListener = (reason: Error | {} | undefined, promise: Promise<any>) => void; | |||
There was a problem hiding this comment.
Are you sure about this?
If I execute Promise.reject([ 1, 2 ]); the reason is [ 1, 2 ].
There was a problem hiding this comment.
The empty object type ({}) refers to the type in your example. You can think of the empty object type as similar to any but, unlike any, it doesn't disable type checking. The user may validate and narrow the type with user-defined type guards.
More information about {} vs any here:
- allow narrowing from any microsoft/TypeScript#9999 (comment)
- Suggestion: stricter type for promise rejection value/reason microsoft/TypeScript#17440
Hopefully one day we'll have a true unknown type: microsoft/TypeScript#10715
There was a problem hiding this comment.
As an example, currently the any type allows this without any errors:
(reason: any) => {
reason.foo.bar // no error :-(
}By changing any to {}, we do get an error. The user must then validate and narrow the type in order to use it. This is of course much more type safe:
const checkIsObjectWithFoo = (value: {}): value is { foo: { bar: {} } } => 'foo' in value;
(reason: {}) => {
reason.foo.bar // error :-)
}
(reason: {}) => {
checkIsObjectWithFoo(reason).foo.bar // no error :-)
}There was a problem hiding this comment.
That's nice, thanks!
I think | null is needed as Promise.reject(null); is possible and null seems to not match {}.
There was a problem hiding this comment.
By changing any to {}, we do get an error.
And that's the problem. There are probably some project relying on the current behavior and would break after this change.
Is it intended to break consumers in a patch release?
If you want that extra type safety, you can always annotate the parameter of your callback.
There was a problem hiding this comment.
Perhaps we can hold off making this change until the next major release? I don't know how those get coordinated, mind you. Is there one planned/upcoming, do you know?
There was a problem hiding this comment.
Hi @ajafff, let me know what you think to my previous comment!
There was a problem hiding this comment.
My learning here is to never ever add any in typings as there is no way to get rid of it without the risk to break something. Versions of typings are dependent on versioning of node therefore no chance to reflect this in the version.
types/node/index.d.ts
Outdated
| @@ -510,7 +510,7 @@ declare namespace NodeJS { | |||
| type ExitListener = (code: number) => void; | |||
| type RejectionHandledListener = (promise: Promise<any>) => void; | |||
| type UncaughtExceptionListener = (error: Error) => void; | |||
| type UnhandledRejectionListener = (reason: any, promise: Promise<any>) => void; | |||
| type UnhandledRejectionListener = (reason: Error | {} | undefined, promise: Promise<any>) => void; | |||
There was a problem hiding this comment.
By changing any to {}, we do get an error.
And that's the problem. There are probably some project relying on the current behavior and would break after this change.
Is it intended to break consumers in a patch release?
If you want that extra type safety, you can always annotate the parameter of your callback.
|
@OliverJAsh 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. Thank you! |
|
@OliverJAsh waiting for the next major version sounds like a good idea. Node.js v10.0.0 is scheduled for 2018-04-24 |
|
Since you're a listed author and the build passed, this PR is fast-tracked. A maintainer will merge 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! |
|
@OliverJAsh is this mergeable now, or do we have to wait? |
|
@OliverJAsh 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! |
|
@OliverJAsh apologies but I've turned the bot into a bit of a stickler. We'll need to do a fresh PR |
|
Hi @ajafff, can we get this merged as part of the next breaking release? |
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.