feat: await-able Async methods #1572

aearly · 2018-08-05T23:30:24Z

aearly · 2018-08-05T23:32:41Z

lib/internal/promiseCallback.js

@@ -0,0 +1,19 @@
+const PROMISE_SYMBOL = Symbol('promiseCallback')
+
+function promiseCallback () {


This is the older strategy. It's a bit more clunky, but doesn't add another frame to the call stack for each method.

I had to use this in a couple places where the awaitify wrapper couldn't work (usually due to optional arguments).

aearly · 2018-08-05T23:32:58Z

lib/internal/awaitify.js

+        })
+    }
+
+    awaitable[Symbol.toStringTag] = 'AsyncFunction'


This is a bit of a hack, not sure if we should do this.

aearly · 2018-08-05T23:33:49Z

lib/internal/awaitify.js

+    }
+
+    awaitable[Symbol.toStringTag] = 'AsyncFunction'
+    awaitable[ASYNC_FN] = true


I intend to use this so we can avoid an additional layer of wrapping in wrapAsync.

aearly · 2018-09-03T08:23:28Z

All the collection methods and relevant control flow methods are now awaitable. However, I'm running into some problems with the Utils methods.

Most of the utils methods are wrappers, e.g. they wrap an async function and return an async function. We use initialParams a lot do to this, it always assumes a callback will be the last argument. However, if we want to make these wrapped functions conditionally awaitable, there are some ambiguities we have to smooth over. We can't know whether the args passed to the wrapped function include a callback or not.

We have avoided looking at fn.length in the past, because it is unreliable, and is 0 for any type of wrapped function or function with rest parameters.
However, we could work in an arity parameter to each method (like I've done for retryable in this PR), defaulting to function.length. If the arity is 0 or undefined, we can just not support awaiting those functions.

The other option is to wrap async functions differently than callback functions. If you pass an async function in to e.g. timeout, you'd get a promise-returning function always. (Currently, we make the wrapAsync() the async function, making it use callbacks internally). This would be a lot of work, as we would have to provide alternate implementations of each utils method, but there might be some cleverness to smooth it over with initialParams.

Or we just punt on this completely. You can't await wrapped functions. This would be a bummer, because await timeout(myFn, 1000)(...args) is pretty powerful.

aearly · 2018-09-17T05:55:50Z

@megawac @hargasinski I would love to get a review on this before merging. In regards to my above comment about the wrapper Utils methods, I'm thinking we can defer a solution there until later (temporary punt).

hargasinski · 2018-09-20T03:26:43Z

@aearly really sorry for the delay. These past few weeks have busy for me, and the PR ended up getting drowned out by other things. Thanks for the reminder! I will review it this weekend. I want to spend some time playing around with it, just to make sure I understand it fully.

megawac

So I reviewed this a couple weeks ago to make sure the code made sense. The reason I didn't approve at the time was (and is) I'm not entirely sure such a change is necessary. Currently if the user wants to use the new async/await syntax they can call utils.promisify(myFavouriteAsyncMethod) or even promisify async.

I can definitely can see the draw of having this done by default I'm just trying to convince myself that this should be done on the methods proper or via a wrapper (e.g. import async from async/awaitable;

megawac · 2018-09-20T13:35:59Z

lib/internal/once.js

        if (fn === null) return;
        var callFn = fn;
        fn = null;
        callFn.apply(this, args);
-    };
+    }
+    Object.assign(wrapper, fn)


What is the purpose of this change?

So static properties attached to the fn get attached to the wrapper, namely the PROMISE_SYMBOL attached to promiseCallbacks.

aearly · 2018-09-20T18:33:04Z

@hargasinski Don't sweat it, after all, this project is run on donated time.

@megawac I hear your concerns about whether we should do this by default -- this is a rather significant change. True, users can promisify methods, but I'm thinking about developer convenience. Already, people are omitting the final callback on an Async method and being surprised a promise is not returned. Several other popular libraries already work like this.

async/await is a very fundamental shift in how async code can work in JavaScript, and IMO it finally obsoletes callback-based approaches. It feels weird to not give first-class support to it -- I feel we're somewhat obligated to because we control the package named async on npm. Newer users of JavaScript are going to become more and more confused about the purpose of this library if we don't make some changes.

This next major version seems like a good point to throw more weight behind async/await-based approaches.

megawac · 2018-09-20T20:04:28Z

How does this affect performance Alex? If its a significant hit my vote will be to seperate the asyncified and original methods into 2 files. I'd be completely fine with the async compatible version being our default export.

I know several people who use our library due to it's performance for intensive use cases so I'm adamant about maintaining good benchmarks :)

aearly · 2018-09-20T20:34:47Z

There was no significant difference in the benchmarks on node 8 or 10. I was concerned the extra wrapper would slow things down, but that is not the case.

hargasinski · 2018-09-24T04:12:29Z

This is some really awesome work! The change look good to me.

Personally, I'm in favour of it. I think there has been enough interest/requests (#439, #956, promise-async, koa-async, async-q) to justify adding it. Also, looking over past issues, it seems that historically we've been against it because promises weren't implemented natively. Now that they are, and with significant parts of the community moving towards promise-based approaches, I think this change makes sense for us.

make each and family awaitable

0d0c6d1

aearly mentioned this pull request Aug 5, 2018

[wip] POC for await-able Async functions #1526

Closed

aearly commented Aug 5, 2018

View reviewed changes

aearly added 2 commits August 5, 2018 16:42

dont pretend they're AsyncFunctions

309dae6

check errors

832159b

aearly mentioned this pull request Aug 23, 2018

New release? #1574

Closed

aearly added 21 commits September 2, 2018 14:40

ensure function name is preserved somehow

208ef4d

awaitable concat

0c74934

awaitable detect

6e07f6c

awaitable every/filter

68363dc

awaitable groupBy

ab7ccd1

awaitable map/mapValues

e5f01b8

awaitable reduce

94c411c

awaitable reject

ed2eb6c

awaitable some

f12ce39

awaitable transform

7df6325

awaitable times

4c53589

awaitable auto

4a33ff9

awaitable compose/seq

39dd905

awaitable whilst/until (lol)

01292f5

awaitable forever

3a30a9f

awaitable parallel/race

0174f3f

awaitable retry

f643878

awaitable series (lol)

4dbafba

awaitable tryEach

105fd5d

awaitable waterfall (lol)

31e68c3

lint

5f71f25

aearly requested review from hargasinski and megawac September 3, 2018 08:24

cleanup, remove noop and unused internal functions

aff6123

This comment has been minimized.

Sign in to view

aearly changed the title ~~[wip] await-able Async methods~~ feat: await-able Async methods Sep 17, 2018

megawac reviewed Sep 20, 2018

View reviewed changes

hargasinski approved these changes Sep 24, 2018

View reviewed changes

aearly merged commit 8aecf10 into master Oct 1, 2018

aearly deleted the awaitable branch October 1, 2018 00:00

aearly added this to the 3.0 milestone Oct 1, 2018

yume-chan mentioned this pull request Aug 14, 2019

[async] add Promise return values DefinitelyTyped/DefinitelyTyped#37605

Closed

9 tasks

dependabot bot mentioned this pull request Mar 16, 2021

Bump async from 2.6.3 to 3.2.0 dudleycarr/nsqjs#314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: await-able Async methods #1572

feat: await-able Async methods #1572

aearly commented Aug 5, 2018

aearly Aug 5, 2018

aearly Sep 3, 2018

aearly Aug 5, 2018

aearly Aug 5, 2018

aearly commented Sep 3, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aearly commented Sep 17, 2018

hargasinski commented Sep 20, 2018

megawac left a comment

megawac Sep 20, 2018

aearly Sep 20, 2018

aearly commented Sep 20, 2018

megawac commented Sep 20, 2018

aearly commented Sep 20, 2018

hargasinski commented Sep 24, 2018

		@@ -0,0 +1,19 @@
		const PROMISE_SYMBOL = Symbol('promiseCallback')

		function promiseCallback () {

feat: await-able Async methods #1572

feat: await-able Async methods #1572

Conversation

aearly commented Aug 5, 2018

aearly Aug 5, 2018

Choose a reason for hiding this comment

aearly Sep 3, 2018

Choose a reason for hiding this comment

aearly Aug 5, 2018

Choose a reason for hiding this comment

aearly Aug 5, 2018

Choose a reason for hiding this comment

aearly commented Sep 3, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aearly commented Sep 17, 2018

hargasinski commented Sep 20, 2018

megawac left a comment

Choose a reason for hiding this comment

megawac Sep 20, 2018

Choose a reason for hiding this comment

aearly Sep 20, 2018

Choose a reason for hiding this comment

aearly commented Sep 20, 2018

megawac commented Sep 20, 2018

aearly commented Sep 20, 2018

hargasinski commented Sep 24, 2018