Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async recipes #307

Merged
merged 15 commits into from
Feb 2, 2019
Merged

Async recipes #307

merged 15 commits into from
Feb 2, 2019

Conversation

mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Feb 2, 2019

Localized branch of #305, implements #302

Remaining work:

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

@coveralls
Copy link

coveralls commented Feb 2, 2019

Coverage Status

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

@aleclarson
Copy link
Member

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
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@aleclarson
Copy link
Member

@aleclarson
Copy link
Member

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

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@aleclarson
Copy link
Member

Also, the types for produce require no changes. 🎉

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@aleclarson
Copy link
Member

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

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@aleclarson
Copy link
Member

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
Copy link
Member

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

@aleclarson
Copy link
Member

aleclarson commented Feb 2, 2019

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
Copy link
Member

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

@mweststrate
Copy link
Collaborator Author

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

@aleclarson
Copy link
Member

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

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@aleclarson
Copy link
Member

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

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@aleclarson
Copy link
Member

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
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@aleclarson
Copy link
Member

aleclarson commented Feb 2, 2019

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

@aleclarson
Copy link
Member

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

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

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@mweststrate mweststrate merged commit 0f76f98 into master Feb 2, 2019
@aleclarson
Copy link
Member

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aleclarson
Copy link
Member

You stole all the credit by squashing it. 😓

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 2, 2019 via email

@mweststrate
Copy link
Collaborator Author

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

@aleclarson
Copy link
Member

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

@mweststrate
Copy link
Collaborator Author

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

@mweststrate
Copy link
Collaborator Author

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

@aleclarson aleclarson deleted the async-recipes branch February 3, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants