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

core: improve Deferred typings #10455

Merged
merged 2 commits into from
Nov 24, 2021
Merged

core: improve Deferred typings #10455

merged 2 commits into from
Nov 24, 2021

Conversation

paul-marechal
Copy link
Member

For some reason Deferred objects allow you to resolve them with
undefined even though the corresponding promise only mentions T.

Remove the optional value when resolving.

Default to void if T is unspecified.

How to test

Everything should build and work as before.

Review checklist

Reminder for reviewers

For some reason `Deferred` objects allow you to resolve them with
`undefined` even though the corresponding promise only mentions `T`.

Remove the optional `value` when resolving.

Default to `void` if `T` is unspecified.
@paul-marechal paul-marechal added the quality issues related to code and application quality label Nov 24, 2021
@paul-marechal
Copy link
Member Author

I noticed this oddity while reviewing #10333 (comment)

We shouldn't be able to resolve with undefined if T doesn't include it explicitly.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

Definitely doesn't make any sense to allow undefined.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Vincent Fugnitto <[email protected]>
@paul-marechal paul-marechal merged commit 134dcf2 into master Nov 24, 2021
@paul-marechal paul-marechal deleted the mp/deferred-typing branch November 24, 2021 18:58
@github-actions github-actions bot added this to the 1.20.0 milestone Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants