fix(core/types): Promise.withResolvers: Unmark callback param as optional

[jsejcksn]

See discussion on merged commit: 1d19b10#r131604700

[bartlomieju]

Thanks!

[bartlomieju]

@jsejcksn I think this is not not quite right, with this change it's not possible to resolve without a value:

[cron_test 002.95] TS2554 [ERROR]: Expected 1 arguments, but got 0.
[cron_test 002.95]     promise1.resolve();
[cron_test 002.95]              ~~~~~~~~~
[cron_test 002.95]     at file:///Users/ib/dev/deno/cli/tests/unit/cron_test.ts:216:14
[cron_test 002.95]
[cron_test 002.95]     An argument for 'value' was not provided.
[cron_test 002.95]         withResolvers<T>(): { promise: Promise<T>, resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void };
[cron_test 002.95]

While the JS API accepts it without a problem:

const d = Promise.withResolvers();
d.resolve();
await d.promise;

[jsejcksn]

@jsejcksn I think this is not not quite right, with this change it's not possible to resolve without a value

@bartlomieju That's the correct, expected behavior. In TypeScript void is the required generic type for invocation with implicit undefined (missing argument) or explicit undefined. The existing Promise constructor callback functions work the same way:

TS Playground

example.ts:

new Promise((resolve) => {
           //^? (parameter) resolve: (value: unknown) => void
  resolve(); /* Error:
  ~~~~~~~~~
  Expected 1 arguments, but got 0. Did you forget to include 'void' in your type argument to 'Promise'? (2794) */
});

new Promise<void>((resolve) => {
                 //^? (parameter) resolve: (value: void | PromiseLike<void>) => void
  resolve(); // Ok
});

declare global {
  interface PromiseConstructor {
    withResolvers<T>(): {
      promise: Promise<T>;
      resolve: (value: T | PromiseLike<T>) => void;
      reject: (reason?: any) => void;
    };
  }
}

{
  const { promise, resolve } = Promise.withResolvers();
                 //^? const resolve: (value: unknown) => void
  resolve(); /* Error:
  ~~~~~~~~~
  Expected 1 arguments, but got 0. (2554) */
  await promise;
}

{
  const { promise, resolve } = Promise.withResolvers<void>();
                 //^? const resolve: (value: void | PromiseLike<void>) => void
  resolve(); // Ok
  await promise;
}

Terminal:

% deno --version
deno 1.38.0 (release, aarch64-apple-darwin)
v8 12.0.267.1
typescript 5.2.2

% deno check example.ts
Check file:///Users/deno/example.ts
error: TS2794 [ERROR]: Expected 1 arguments, but got 0. Did you forget to include 'void' in your type argument to 'Promise'?
  resolve(); /* Error:
  ~~~~~~~~~
    at file:///Users/deno/example.ts:3:3

    An argument for 'value' was not provided.
        new <T>(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): Promise<T>;
                                     ~~~~~~~~~~~~~~~~~~~~~~~~~
        at asset:///lib.es2015.promise.d.ts:31:34

TS2554 [ERROR]: Expected 1 arguments, but got 0.
  resolve(); /* Error:
  ~~~~~~~~~
    at file:///Users/deno/example.ts:26:3

    An argument for 'value' was not provided.
          resolve: (value: T | PromiseLike<T>) => void;
                    ~~~~~~~~~~~~~~~~~~~~~~~~~
        at file:///Users/deno/example.ts:17:17

Found 2 errors.

[jsejcksn]

@bartlomieju Re: the partial error message you included:

[cron_test 002.95] TS2554 [ERROR]: Expected 1 arguments, but got 0.
[cron_test 002.95]     promise1.resolve();
[cron_test 002.95]              ~~~~~~~~~
[cron_test 002.95]     at file:///Users/ib/dev/deno/cli/tests/unit/cron_test.ts:216:14
[cron_test 002.95]
[cron_test 002.95]     An argument for 'value' was not provided.
[cron_test 002.95]         withResolvers<T>(): { promise: Promise<T>, resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void };
[cron_test 002.95]

All the checks passed for this PR before it was merged, so I'm not sure how that was run. It looks like that came from here:

deno/cli/tests/unit/cron_test.ts

Lines 200 to 221 in fdb4953

	Deno.test(async function overlappingExecutions() {
	Deno.env.set("DENO_CRON_TEST_SCHEDULE_OFFSET", "100");
	let count = 0;
	const promise0 = deferred();
	const promise1 = deferred();
	const ac = new AbortController();
	const c = Deno.cron("abc", "*/20 * * * *", async () => {
	promise0.resolve();
	count++;
	await promise1;
	}, { signal: ac.signal });
	try {
	await promise0;
	} finally {
	await sleep(2000);
	promise1.resolve();
	ac.abort();
	await c;
	}
	assertEquals(count, 1);
	});

And it looks like the values promise0 and promise1 on lines 204–205 are the return type of the deferred function from denoland/deno_std here:

https://github.com/denoland/deno_std/blob/b23a76a47adaf63b5493761a87e6dd0dc0de0bb8/async/deferred.ts

I'm guessing that the changes which need to be made are to annotate the void generic at the call sites like this:

-  const promise0 = deferred();
-  const promise1 = deferred();
+  const promise0 = deferred<void>();
+  const promise1 = deferred<void>();

[bartlomieju]

@jsejcksn The change I applied was changing deferred() to Promise.withResolvers(). For now I don't want to touch deferred's implementation.

I guess it's okay to change all call sites in cli/tests/unit that used const p = deferred and p.resolve() to explicitly pass undefined as an argument.