fix(std): Converting Deferred interface to class

[mayankagarwals]

Converts Deferred interface in std/async to class. The deferred function can be removed then.

[lucacasonato]

As discussed in std-wg. Could you remove the TODO comment, and update the tests?

[mayankagarwals]

Yeah, sure thing. I just wanted to create a draft PR to hear everyone's opinions.

[mayankagarwals]

Also there is a problem with current implementation.
The snippet

const p = deferred();
p.resolve()

would actually instantiate a deferred<{}>. According to microsoft/TypeScript#39817, unless promise type is void, resolve should pass an argument of type T. In current implementation due to some bug, this is bypassed. The fault is fixed in this PR.

[mayankagarwals]

CC @ry @lucacasonato

[ry]

Is it necessary to put the <void> in here?

[mayankagarwals]

Yes, or else the default type is {} and mandates a parameter of type {} when resolve is called. This fix was introduced in a later version of typescript though (microsoft/TypeScript#39817).
Edit: This was introduced in the latest version of typescript released last month.

[bartlomieju]

#7573 (comment)

[ry]

Writing new Deferred<void> is fairly obnoxious. Much longer than deferred. We should reconsider this change.

[nayeemrmn]

@ry Additionally, extending Promise has some annoying behaviours which forces it to accept an executor function, which deferred() did not. It makes .then() calls also return Deferred instances which makes no sense.

[bartlomieju]

I'm actually pretty happy with current implementation, I think we should only change Object.assign() bit to directly assign resolve and reject to promise

[mayankagarwals]

@ry @nayeemrmn, I think we'll need to explicitly write deferred<void> too. Not mentioning correct type in https://github.com/denoland/deno/blob/master/std/async/deferred.ts#L8 allows current behaviour but has the downside of not following latest promise structure. But that can be fixed by adding right type to current implementation too.

I don't mind the current implementation either but addition to @bartlomieju comment, I think we should consider following typescript's promise structure

[mayankagarwals]

Looks like this is an anti-pattern . Also, refer https://github.com/denoland/deno/issues/8405#issuecomment-739420300.
We should consider refactoring it out of the code.

[hayd]

@iammack it can be useful, in fact in your anti-pattern link it says:

In Deferred anti-pattern, "deferred" objects are created for no reason, complicating code.

So when should deferred be used?
Well simply, when you have to.

Sometimes you have to.

[mayankagarwals]

@hayd, it would be helpful if you could provide an example where using deferred is the only option. Regarding the example provided in the article, I think this snippet can be used instead:

function delay(ms){
	return new Promise((resolve, reject) => {
		setTimeout(resolve, ms);
	});
}
// const startTime = new Date();
const p = delay(2000);
// await p;
// const endTime = new Date();
// console.log(endTime - startTime);

[hayd]

@iammack An example where this allows for an intuitive algorithm is a connection pool, where the async method returns a Connection object, which is then released (put back into the connection pool) once it's been used. If there isn't a Connection object available it waits until one has been released. (e.g. here.)

[mayankagarwals]

Apologies for the late reply, been stuck in some academics related work. I think @benjamingr should weigh in here, he has better knowledge of this :)

[benjamingr]

My opinion (was) and still is that going against the standard by not using the provided construction method (the promise constructor) that is designed to be throw-safe and instead using the old (and removed from the spec) Deferred is a mistake.

I would just refactor things to the promise constructor or better - avoid it in most places altogether.

[benjamingr]

Of course - Deno is entirely at liberty to introduce its own abstractions rather than use JavaScript's for promise construction. Just in case the above implies differently. It's just kind of weird to pick the variant TC39 ruled out for being unsafe and foot-gunny.

[bartlomieju]

My opinion (was) and still is that going against the standard by not using the provided construction method (the promise constructor) that is designed to be throw-safe and instead using the old (and removed from the spec) Deferred is a mistake.

I would just refactor things to the promise constructor or better - avoid it in most places altogether.

@benjamingr could you elaborate on this one? What would you suggest instead?

[benjamingr]

@benjamingr could you elaborate on this one? What would you suggest instead?

Sure:

var d = Deferred();
someAsyncOp(() => {
  d.resolve();
});
return d.promise;

Becomes:

return new Promise((resolve) => someAsyncOp(() => resolve()));

Or better - you can call util.promisify (from node/std or deno can ship one itself) and just use the promises returning version.

[nayeemrmn]

@benjamingr What about, say, ServerRequest::done, which is resolved in a method?

I'm also against overriding tc39's intentional design without good domain-specific reasons. But I think a lot of the call sites in Deno's codebase would just be replaced by an inline version of deferred().

[benjamingr]

@benjamingr What about, say, ServerRequest::done, which is resolved in a method?

When you explicitly need indirection like that - you have no choice but to pick a way to "escape" the promise constructor. I would not use a deferred here I'd use a more specific type that doesn't expose the capability of resolving the promise.
The current API (IIUC) allows consumers to call request.done.resolve(true) and cause a problematic state in the code.

Probably just inline it:

• Make done a promise (and not a deferred).
• Store its resolve as a private property on the request.
• Resolve it (and not the deferred);

Note this is still not great but I'm not sure what to do - I will ask Domenic an some people who were in TC39 when the decision happened what they think.

P.S. why does that code resolve with an Error object rather than reject to signal an error has occured in that request? Is that because of unhandled rejections?

[benjamingr]

What about something like:

• Create a detachPromise type that returns a promise that tracks another promise when it is available.
• Track the promise of respond and resolve it with that.

I don't mind writing some code if the gist of it is unclear.

[bartlomieju]

Deno standard library was moved to denoland/deno_std, please reopen your PR there.