Remove optionality for Promise resolve callback

[Ron Buckton (rbuckton)]

This makes value required in the resolve callback for a new Promise. This shouldn't affect the ability to call resolve() without an argument when creating a new Promise<void>, as we already have logic in place that allows us to parameters whose types are void as optional.

Fixes #36749

[Ron Buckton (rbuckton)]

TypeScript Bot (@typescript-bot) run dt
TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the extended test suite on this PR at b198eb6. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized Definitely Typed test suite on this PR at b198eb6. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized community code test suite on this PR at b198eb6. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[Ron Buckton (rbuckton)]

Ryan Cavanaugh (@RyanCavanaugh), Wesley Wigham (@weswigham), Daniel Rosenwasser (@DanielRosenwasser): This one is interesting because its an improvement for TypeScript code, but results in "breaks" for JavaScript code.

In TS, the expectation is that new Promise<number>(resolve => resolve()) should be an error, and that new Promise<void>(resolve => resolve()) shouldn't, and that's fine. However, in the JS projects we use as part of the user test suite, they might just use new Promise(resolve => resolve()) and we'll infer the type argument as unknown. This results in "breaks" in JS code because unknown doesn't follow the same rule for parameter optionality that void does.

Removing optionality for resolve is a fairly small fix that in general is an improvement; however, I'm not sure whether we would consider the JavaScript "breaks" as enough of a breaking change to hold off on merging this until 4.1, so some feedback here would be appreciated.

[Wesley Wigham (weswigham)]

Hm.... I mean, unknown includes void and undefined in its domain; perhaps the rule allowing the parameter to be dropped should be extended to types-which-possibly-include-void-in-their-domain?

[Ron Buckton (rbuckton)]

Daniel Rosenwasser (@DanielRosenwasser), Ryan Cavanaugh (@RyanCavanaugh): I'm still interested in whether this is considered a "break" and if it should wait for 4.1, or if we should consider Wesley Wigham (@weswigham)'s suggestion to extend the void optionality to unknown and undefined (and maybe any) for JavaScript only (in which case this might be feasible for >=4.0.1 <4.1)

[Daniel Rosenwasser (DanielRosenwasser)]

This late in the release, I'd really prefer waiting until 4.1 for anything that feels close to a breaking change.

[ExE Boss (ExE-Boss)]

This shouldn't affect the ability to call resolve() without an argument when creating a new Promise<void>, as we already have logic in place that allows us to parameters whose types are void as optional.

Does that mean that #29131 has been fixed?

[Andrew Branch (andrewbranch)]

After discussing it in the design meeting, the suggestion to make trailing unknown parameters optional in JS seems reasonable to me. If there’s concern about it, that behavior could potentially be disabled in --strictNullChecks, where it might be ok to get people to add a /** @type {Promise<void> */ (or @return, any way of adding the contextual type).

[Ron Buckton (rbuckton)]

TypeScript Bot (@typescript-bot) user test this

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized community code test suite on this PR at ff5009c. You can monitor the build here.

[Ron Buckton (rbuckton)]

From the user tests:

office-ui-fabric

@uifabric/build: gulp/tasks/browserAdapters.ts:58:7 - error TS2554: Expected 1 arguments, but got 0.
@uifabric/build: 58       resolve();
@uifabric/build:          ~~~~~~~~~
@uifabric/build:   ../node_modules/typescript/lib/lib.es2015.promise.d.ts:33:34
@uifabric/build:     33     new <T>(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): Promise<T>;
@uifabric/build:                                         ~~~~~~~~~~~~~~~~~~~~~~~~~
@uifabric/build:     An argument for 'value' was not provided.

This is due to the fact that the promise created here has no type argument and is inferred to Promise<unknown>. As this is a .ts file, we do not consider the value optional here, so this error is expected.

xterm.js

src/browser/Terminal2.test.ts(80,11): error TS2554: Expected 1 arguments, but got 0.

This is again due to an untyped Promise in a ts file being inferred to unknown and is expected of this change.

adonis-framework

node_modules/adonis-framework/src/Static/index.js(45,18): error TS2554: Expected 1 arguments, but got 0.

This is due to the fact this is a .js file and our user tests compile js files with --strictNullChecks, so this error is expected. Without --strictNullChecks, #40057 would kick in and allow you to elide the argument.

chrome-devtools-frontend

node_modules/chrome-devtools-frontend/front_end/bindings/FileUtils.js(210,33): error TS2345: Argument of type '(value: any) => void' is not assignable to parameter of type '() => any'.
...
node_modules/chrome-devtools-frontend/front_end/persistence/FileSystemWorkspaceBinding.js(526,106): error TS2345: Argument of type '(value: any) => void' is not assignable to parameter of type '() => any'.
...
node_modules/chrome-devtools-frontend/front_end/test_runner/TestRunner.js(28,5): error TS2554: Expected 1 arguments, but got 0.
...
node_modules/chrome-devtools-frontend/front_end/test_runner/TestRunner.js(207,40): error TS2322: Type '(value: any) => void' is not assignable to type '() => void'.
...
node_modules/chrome-devtools-frontend/front_end/test_runner/TestRunner.js(502,107): error TS2345: Argument of type '(value: any) => void' is not assignable to parameter of type '() => any'.
...
node_modules/chrome-devtools-frontend/front_end/test_runner/TestRunner.js(947,58): error TS2345: Argument of type '(value: any) => void' is not assignable to parameter of type '() => void'.
node_modules/chrome-devtools-frontend/front_end/test_runner/TestRunner.js(981,55): error TS2345: Argument of type '(value: any) => void' is not assignable to parameter of type '() => void'.
...
node_modules/chrome-devtools-frontend/front_end/timeline/ExtensionTracingSession.js(20,7): error TS2322: Type '(value: any) => void' is not assignable to type '() => any'.

In these cases, the promises need an appropriate contextual type. Some are typed as Promise<any> as that is the default inference in a .js file, and the resulting resolve callback argument is no longer considered optional and is thus unassignable to the jsdoc type function(). In one case, the promise is contextually typed to Promise<undefined> but the jsdoc type should be rewritten to Promise<void>.

npm

node_modules/npm/lib/auth/legacy.js(12,81): error TS2554: Expected 1 arguments, but got 0.

This is again due to the fact we compile our JS user tests using --strictNullChecks and the promise has no contextual type.

puppeteer

src/common/LifecycleWatcher.ts(137,7): error TS2322: Type '(value: void | PromiseLike<void>) => void' is not assignable to type '() => void'.
src/common/Target.ts(95,21): error TS2322: Type '(value: boolean | PromiseLike<boolean>) => void' is not assignable to type '() => void'.
src/node/BrowserFetcher.ts(548,28): error TS2345: Argument of type '(fulfill: () => void, reject: (Error: any) => void) => void' is not assignable to parameter of type '(resolve: (value: void | PromiseLike<void>) => void, reject: (reason?: any) => void) => void'.
  Types of parameters 'fulfill' and 'resolve' are incompatible.

• LifecycleWatcher.ts - This one is interesting. Even though we consider void to be "optional" for calls, we do not consider it to be optional for assignability purposes.
• Target.ts - This looks like a bug in the user code. The promise is clearly typed as Promise<boolean>, but its resolve callback is being assigned to a field of type () => void, meaning that it is likely never resolved correctly (which is the kind of bug this PR intends to catch).
• BrowserFetcher.ts - As with LifecycleWatcher.ts, this is a case where we don't treat a void parameter as optional for assignability, only for calls.

Daniel Rosenwasser (@DanielRosenwasser), Ryan Cavanaugh (@RyanCavanaugh):

Regarding our existing support for treating trailing void parameters as optional, it seems we have a bug in that we only treat them as optional for calls, but not for assignability purposes. I will file that as a separate issue.

I'm also considering the possibility of giving an await expression whose value is unused the contextual type of void | PromiseLike<void>, for example:

async function foo() {
    // type is `Promise<unknown>`.
    const x = new Promise(() => {});

    // type is `Promise<void>`
   await new Promise(() => {});
}

The result would be that some of the above errors would get the correct contextual type when the result is unused. I can create a separate PR for this case, which we can possibly discuss at the next design meeting.

[Ron Buckton (rbuckton)]

I've filed #40227 for the assignability related issue.

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized Definitely Typed test suite on this PR at 47b29cc. You can monitor the build here.

[Ron Buckton (rbuckton)]

TypeScript Bot (@typescript-bot) run dt

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized Definitely Typed test suite on this PR at 47b29cc. You can monitor the build here.

[Ron Buckton (rbuckton)]

TypeScript Bot (@typescript-bot) run dt

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized Definitely Typed test suite on this PR at 47b29cc. You can monitor the build here.

[Ron Buckton (rbuckton)]

trying again...
TypeScript Bot (@typescript-bot) run dt

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized Definitely Typed test suite on this PR at 47b29cc. You can monitor the build here.

[Ron Buckton (rbuckton)]

One last time, now that hopefully dtslint-runner is fixed...
TypeScript Bot (@typescript-bot) run dt

[TypeScript Bot (typescript-bot)]

Heya Ron Buckton (@rbuckton), I've started to run the parallelized Definitely Typed test suite on this PR at 47b29cc. You can monitor the build here.

[John Firebaugh (jfirebaugh)]

FYI, this is a breaking change for utility functions that externalize the resolve and reject functions passed to the Promise constructor:

export function promiseResolveReject<T = void>(): [Promise<T>, (value?: T | PromiseLike<T>) => void, (reason?: any) => void] {
  let resolve: (value?: T | PromiseLike<T>) => void;
  let reject: (reason?: any) => void;
  const promise = new Promise<T>((resolve_, reject_) => {
    resolve = resolve_;
    reject = reject_;
  });
  return [promise, resolve!, reject!];
}

Error:

../../ts/src/util.ts(14,5): error TS2322: Type '(value: T | PromiseLike<T>) => void' is not assignable to type '(value?: T | PromiseLike<T> | undefined) => void'.
  Types of parameters 'value' and 'value' are incompatible.
    Type 'T | PromiseLike<T> | undefined' is not assignable to type 'T | PromiseLike<T>'.
      Type 'undefined' is not assignable to type 'T | PromiseLike<T>'.

[Ron Buckton (rbuckton)]

Yes, this is a breaking change, but one that is a change in correctness that adds type-safety. The prior definition was unsafe as it allowed you to pass undefined to a resolve for a Promise<number>. Custom logic around resolve will unfortunately need to be updated to ensure correctness.

Above, you would need to change let resolve to let resolve: (value: T | PromiseLike<T>) => void, and the return type of the function to [Promise<T>, (value: T | PromiseLike<T>) => void, (reason?: any) => void]. There is no way for us to do that automatically.

CC Daniel Rosenwasser (@DanielRosenwasser) to document in the 4.1 release notes that this is a breaking change intended to improve type safety.

[James Bromwell (thw0rted)]

I just upgraded to 4.1, and I'm trying to follow the conversation here and at #40231 and #41497. My main problem is that, in code like this:

const p = new Promise<number | undefined>(resolve => {
  if (Math.random() > 0.5) { resolve(1); }
  else { resolve(); }
});

the type of resolve is (value: number | PromiseLike<number | undefined> | undefined) => void rather than (value?: number | PromiseLike<number | undefined>) which means that the call to resolve() with no argument is flagged. I can pass an explicit undefined but this is against our style guide and our linter complains about it. I can't change the Promise to number | void because the caller expects to assign the result to a number | undefined.

I think we're stuck changing our style guide to allow passing undefined explicitly, but I'd like to know what the team's intent was here -- maybe I'm missing something.

[Omar Diab (osdiab)]

I have this code in my codebase:

  return new Promise((resolve, reject) => {
    writeStream.on("finish", () => {
      resolve();
    });
    writeStream.on("error", (error) => {
      reject(error);
    });
  });

it doesn't actually parameterize Promise so I expected it to be fine, but I get the error that i need to provide a value for resolve; I'm walking around it by explicitly saying that this is a Promise<void> but it's def not what I expected, should the Promise generic default to void perhaps? Promise<T = void> { ... } and if you provide a non-void value I believe it would infer correctly anyway.