Async recipes

[mweststrate]

Localized branch of #305, implements #302

Remaining work:

• docs
• createDraft / finishDraft api
• typings create/finish Draft
• typings async produce

[coveralls]

Coverage increased (+0.03%) to 99.723% when pulling dfdc465 on async-recipes into 271cd1b on master.

[aleclarson]

Heads up, I force-pushed to remove the merge commit and reword the createDraft/finishDraft commit so it shows up on the Releases page.

[mweststrate]

Oww sorry, yes I should read up on the new commit / release process. Any recommended link? Op za 2 feb. 2019 14:38 schreef Alec Larson <notifications@github.com:

…

Heads up, I force-pushed to remove the merge commit and reword the createDraft/finishDraft commit so it shows up on the Releases page. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhO5h15iS9Yo9NXlI88RmO49VyG7Vks5vJZTBgaJpZM4afhtr> .

[aleclarson]

Here you go: https://www.conventionalcommits.org/en/v1.0.0-beta.2/ 👍

[aleclarson]

I wonder if createDraft should "tag" its drafts so we can throw if a produce-made draft is ever passed to finishDraft.

[mweststrate]

Good point, Wii add Op za 2 feb. 2019 15:00 schreef Alec Larson <notifications@github.com:

…

I wonder if createDraft should "tag" its drafts so we can throw if a produce-made draft is ever passed to finishDraft. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhEAcBbfVZ3Cu5s75sqbhtML8n4Coks5vJZn3gaJpZM4afhtr> .

[aleclarson]

Also, the types for produce require no changes. 🎉

[mweststrate]

Not for the async version? I would expect to need something like produce<T>(Draft<T> => Promise<void | T>): Promise<T> ? Op za 2 feb. 2019 om 15:11 schreef Alec Larson <notifications@github.com>:

…

Also, the types for produce require no changes. 🎉 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhAVkzuLQEBISN7UHZOA2qQSsRIVrks5vJZymgaJpZM4afhtr> .

[aleclarson]

Ah good point, didn't think of the Promise<void> case. Would that make this a breaking change?

[mweststrate]

No I don't think so, as promises aren't draftable, it doesn't conflict with any existing use case Op za 2 feb. 2019 15:15 schreef Alec Larson <notifications@github.com:

…

Ah good point, didn't think of the Promise<void> case. Would that make this a breaking change? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhLoYMZNS_GECfINz6kpUvqiFRa2iks5vJZ2egaJpZM4afhtr> .

[aleclarson]

While unlikely, some users may have returned a Promise<void> as a "replacement value". If it suddenly becomes a Promise<T>, that might break consumers of that promise. But since the result of a Promise<void> is ignored 99.9% of the time, a better example would be Promise<string | undefined> becoming Promise<string | T> once this PR is merged.

[aleclarson]

I'm updating the tests to use snapshots for error messages and patches. 😎

[aleclarson]

We may want to consider changing the return type of finishDraft to Immutable<T>, because TypeScript cannot infer T from Draft<T>. Thus, the current return type of finishDraft is mutable.

edit: I went ahead with this. 👍

[aleclarson]

Okay, the return type of produce has been updated and simplified! 🚀

[mweststrate]

@aleclarson awesome! I'll proceed with the docs in an iffy, and then we'll should be ready to merge I guess!

[aleclarson]

Since we'll be publishing a major, we must decide if #246 should be addressed, too.

[mweststrate]

Eh.. why no minor? Op za 2 feb. 2019 19:02 schreef Alec Larson <notifications@github.com:

…

Since we'll be publishing a major, we must decide if #246 <#246> should be addressed, too. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhCRSvwvqLoVRn1BYthBLyil1XF2oks5vJdLEgaJpZM4afhtr> .

[aleclarson]

As mentioned here, this PR causes produce to return Promise<string | T> where it previously returned Promise<string | undefined>.

[mweststrate]

But that only changes the static types, not the runtime behavior, correct? (Imho the best practice is not to count these as breaking changes, cause otherwise every time the typings improve, you have a breaking change, while usually it just reveals an already wrong assumption on the consumer) Op za 2 feb. 2019 19:05 schreef Alec Larson <notifications@github.com:

…

As mentioned here <#307 (comment)>, this PR causes produce to return Promise<string | T> where it previously returned Promise<string | undefined>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhAZAi6tSYivue22YdlKV4Wbqqyc1ks5vJdN8gaJpZM4afhtr> .

[aleclarson]

We never said in the docs that returning a promise from a producer is not supported. We just said not to modify the draft in an "async process", which isn't the same thing.

But that only changes the static types, not the runtime behavior, correct?

Nope. Runtime behavior is affected, because we're changing the result of the returned promise. Promises that previously resolved with an undefined value will now resolve with the base state or a modified copy.

[mweststrate]

Ok check! I think #246 can be done separately, as that one requires a real migration Op za 2 feb. 2019 19:16 schreef Alec Larson <notifications@github.com:

…

We never said in the docs that returning a promise from a producer is not supported. We just said not to modify the draft in an "async process", which isn't the same thing. But that only changes the static types, not the runtime behavior, correct? Nope. Runtime behavior is affected, because we're changing the result of the returned promise. Promises that previously resolved with an undefined value will now resolve with the base state or a modified copy. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhBO9FN1l50uDwmYznsZfRXKWBqqSks5vJdX8gaJpZM4afhtr> .

[mweststrate]

At some point we might remove the this=draft in recipes as well. I think it's a useless feature Op za 2 feb. 2019 19:19 schreef Michel Weststrate <mweststrate@gmail.com:

…

Ok check! I think #246 can be done separately, as that one requires a real migration Op za 2 feb. 2019 19:16 schreef Alec Larson ***@***.***: > We never said in the docs that returning a promise from a producer is not > supported. We just said not to modify the draft in an "async process", > which isn't the same thing. > > But that only changes the static types, not the runtime behavior, correct? > > Nope. Runtime behavior is affected, because we're changing the result of > the returned promise. Promises that previously resolved with an undefined > value will now resolve with the base state or a modified copy. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#307 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABvGhBO9FN1l50uDwmYznsZfRXKWBqqSks5vJdX8gaJpZM4afhtr> > . >

[aleclarson]

It's basically useless, but also harmless. Maybe just remove it from the docs? (here)

[aleclarson]

Actually, nevermind. We should remove it so you can do the following:

const context = {
  foo: 1,
  bar: produce(function(draft) {
    draft.foo = this.foo
  })
}

[aleclarson]

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[aleclarson]

You stole all the credit by squashing it. 😓

[mweststrate]

Darn, thought that was required for semantic-release. If you want I force-push a normal merge. Op za 2 feb. 2019 20:55 schreef Alec Larson <notifications@github.com:

…

You stole all the credit by squashing it. 😓 — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhAV4AdRd7jAxDsMKSGBs2RL7WcV4ks5vJe0dgaJpZM4afhtr> .

[mweststrate]

Done, an cancelled the resulting build. Hopefully this doesn't mess up the next semantic release computation :-P.

[aleclarson]

If it does, I think we just need to manually push the next version to fix it. Thanks! 👍

[mweststrate]

Good point, I'll release a no-op 2.0.1 to make sure it won't be forgotten

[mweststrate]

I force pushed a new tag v2.0.0, I'm pretty sure that should already fix it, release overview now links to the correct commit as well