Skip to content

Consistently use Promise<void> instead of Promise<undefined> #1598

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

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

lucacasonato
Copy link
Contributor

@lucacasonato lucacasonato commented Jul 19, 2023

lib.dom.d.ts ubiquitously uses Promise<void> for promises not resolving
to undefined. It has done this for a long time.

When the generator was written, there were 0 or 1 (I can't quite tell)
places where this type of promise occured outside of a function return
value. However, Web Streams are now widely supported, and they have
properties that are promises that resolve to no value. These are
currently typed as Promise<undefined>. This commit changes them to be
typed as Promise<void>, to align with the rest of lib.dom.d.ts.

There are 5 readonly properties that are impacted by this change.

@github-actions
Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@lucacasonato
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Deno Land Inc"

lib.dom.d.ts ubiquitously uses Promise<void> for promises not resolving
to undefined. It has done this for a long time.

When the generator was written, there were 0 or 1 (I can't quite tell)
places where this type of promise occured outside of a function return
value. However, Web Streams are now widely supported, and they have
properties that are promises that resolve to no value. These are
currently typed as Promise<undefined>. This commit changes them to be
typed as Promise<void>, to align with the rest of lib.dom.d.ts.

There are 5 readonly properties that are impacted by this change.
@saschanaz
Copy link
Contributor

Can you modify the description as it all says just Promise, I believe you intended to say Promise<something>.

I'm not sure this is the right direction btw, shouldn't we do the reverse way and do Promise<undefined> everywhere? cc @sandersn

@lucacasonato
Copy link
Contributor Author

@saschanaz indeed, had to escape the < and > chars

@saschanaz
Copy link
Contributor

saschanaz commented Jul 19, 2023

Does this change affect real code btw? Having Promise<undefined> should be okay since microsoft/TypeScript#53092.

@lucacasonato
Copy link
Contributor Author

It has a compat issue with lib.deno.web.d.ts, as it uses Promise<void> throughout

@sandersn
Copy link
Member

sandersn commented Jul 20, 2023

In the absract, switching to Promise<undefined> would be the right thing, but both the weight of backward compatibility and the special cases for returning void mean that it's likely to break something.

I'm not at my desk right now but when I get back I'll try an experiment of manually replacing Promise<void> with Promise<undefined> in the std lib. I see 93 uses.

Thinking about it a bit more, most of those 93 uses are the return types of async functions. And I suspect that environments like node and deno have lots more uses like that. So maybe it's better to give these 5 properties worse types for consistency.

@sandersn
Copy link
Member

sandersn commented Jul 20, 2023

Looking at the results so far, I see three categories of break:

  1. Promise<undefined> isn't assignable to explicit type annotations : Promise<void>
  2. In an async function, returning a Promise<void> counts as void for the purposes of classifying a function as needing to return a value from each code path. Returning a Promise<undefined> from one branch means that other branches need to return a value as well.
  3. Code that extends DOM types in specific ways isn't assignable because the overrides have explicit Promise<void> types. I didn't take time to understand the details; see the DT results.

@saschanaz
Copy link
Contributor

2. In an async function, returning a Promise<void> counts as void for the purposes of classifying a function as needing to return a value from each code path. Returning a Promise<undefined> from one branch means that other branches need to return a value as well.

This sounds like a bug, both should work same, right?

But it does sound like we should compromise here.

@Renegade334
Copy link
Contributor

This raised its head again with microsoft/TypeScript#60987, where certain third-party polyfills (for webcodecs in particular) have properties typed as Promise<void> but where those missing interfaces are now present in the DOM libs with properties typed as Promise<undefined> as per the IDL, causing errors.

Was the verdict here WONTFIX?

@saschanaz
Copy link
Contributor

Err, I think this has been just forgotten. Let's merge this. LGTM.

@github-actions github-actions bot merged commit 1268ed6 into microsoft:main Jan 26, 2025
Copy link
Contributor

Merging because @saschanaz is a code-owner of all the changes - thanks!

saschanaz added a commit to saschanaz/types-web that referenced this pull request Jan 27, 2025
github-actions bot pushed a commit that referenced this pull request Jan 27, 2025
Co-authored-by: saschanaz <saschanaz@users.noreply.github.com>
sandersn added a commit to sandersn/TypeScript that referenced this pull request Jan 29, 2025
This is going into nightly to make sure that
microsoft/TypeScript-DOM-lib-generator#1598

is goodto ship for the RC.
@darksabrefr
Copy link

These changes have to be reflected in Node's types too, because they use Promise<undefined>, making them not compatible anymore since TS 5.8 was released : https://github.com/DefinitelyTyped/DefinitelyTyped/blob/515f950351b890f8c45bbfcc793cdc9598da67e8/types/node/stream/web.d.ts#L100

@darksabrefr
Copy link

Discussion opened here: DefinitelyTyped/DefinitelyTyped#72102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants