feat: support async recipes

[aleclarson]

Drafts won't be revoked until the returned promise is fulfilled or rejected.

The readme needs to be updated to reflect this. Anyone willing to contribute here?

As suggested by @Yurickh in #302 (comment)

[aleclarson]

I've added tests for the two basic cases:

• modifying the draft before and after the recipe function has returned
• awaiting a rejected promise

If anyone has more test ideas, please speak up!

[coveralls]

Coverage increased (+0.02%) to 99.714% when pulling 5a99c88 on aleclarson:promise into 271cd1b on mweststrate:master.

[mweststrate]

Great stuff! Will review in details next week, but the Promise support is an extremely useful feature! I can update the typings and docs as well. I might add a bit of refactoring on top of this, to also expose the lower level api as proposed in #302,

• docs
• typings
• low level api

P.S.: you do have the permissions to directly create branches on this repository, which makes it possible to both commit to the same branch :)

[aleclarson]

P.S.: you do have the permissions to directly create branches on this repository, which makes it possible to both commit to the same branch :)

True! I would do that, but I would need to reopen this PR. Note that "Allow edits from maintainers" is enabled! ;)

[mweststrate]

questions that just popped up in my mind: does this affect isDraft / original / recursive produce calls if they are invoked after the first await?

[aleclarson]

Definitely not for isDraft and original, but we do need tests for recursive produce calls after await.

[aleclarson]

For recursive use after await, one bug is that the nested produce call will return an auto-frozen copy even when it contains a draft owned by another produce call.

During the finalize phase, we need to track how many unowned drafts will exist in the returned copy. We would only be able to auto-freeze the result if no unowned drafts are found.

edit: Working on a fix for this..

[aleclarson]

Note: With async recipe functions, it remains unsafe to "hoist" drafts out of the recipe, since they can be revoked at any time.

[mweststrate]

also note that in that sense it also can' t be used to solve the lowlevel proposal in 302; as finalizing would always happen on at least one tick later; when the promise settles Op do 31 jan. 2019 om 16:37 schreef Alec Larson <notifications@github.com>:

…

Note: When using an async recipe function, you're free to "hoist" drafts out of their recipe function, since they'll definitely be finalized when the promise is resolved. But it remains unsafe to do the same with a synchronous recipe function. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#305 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhG-CgmvZi0yduz15Fn6tEJmqJp3Cks5vIw2dgaJpZM4abRWX> .

[aleclarson]

as finalizing would always happen on at least one tick later; when the promise settles

I don't think so, because you're not required to use promises when deferring the finalize phase. I've separated that logic into the processResult method.

[mweststrate]

ah, that is awesome @aleclarson!

[aleclarson]

Note: With the current implementation, you wouldn't be able to call finishDraft more than once on any draft related to a single createDraft call, because they all share the same ImmerScope instance.

[aleclarson]

The issue mentioned in #305 (comment) has been fixed. 🎉

[aleclarson]

Note: With e280946, unowned drafts will only prevent auto-freezing of affected "branches". Branches which contain no unowned drafts will be safely auto-frozen! 👍

Previously, a nested produce call would never auto-freeze anything in its result.

[aleclarson]

@mweststrate Node 6 doesn't support async/await, so the tests are failing. Maybe we should drop Node 6 support? :P

[Yurickh]

@mweststrate Node 6 doesn't support async/await, so the tests are failing. Maybe we should drop Node 6 support? :P

THAT would be a breaking change after all haha

[aleclarson]

@Yurickh Yup! 😆 I decided against that silly idea.

We can revert 5a99c88 when the time comes!

[mweststrate]

Closed MR in favor of #307 (which is the same, but on this repo)

[aleclarson]

@mweststrate Were you having trouble editing this PR? Or just want to avoid adding another git remote?

[mweststrate]

The latter, wasn't sure yet whether to use one or two branches. But changes are so small so far I'll keep it as one Op za 2 feb. 2019 14:34 schreef Alec Larson <notifications@github.com:

…

@mweststrate <https://github.com/mweststrate> Were you having trouble editing this PR? Or just want to avoid adding another git remote? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#305 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhNoMwwT5f8jhu3U98kjvZ__oTeI9ks5vJZPogaJpZM4abRWX> .