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

Initial exploration into an async check #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidnormo
Copy link

@davidnormo davidnormo commented Mar 21, 2018

Following on from #45

All credit for the async quick-check implementation goes to promesa-check ⭐️

This PR adds this behind a new function: checkAsync. This acts exactly the same as check except that the property function needs to return a promise.

Ideally I'd prefer check to be able to accept both sync and async properties, which I may work on, but thought I'd share this for some early feedback

@leebyron
Copy link
Owner

Thanks for doing this! Very cool to see the async check implementation out there, that definitely simplifies some things.

Some next steps I can see:

  • Looks like check and checkAsync are largely the same - perhaps there is some code reuse to be had?
  • Tests are pretty sparse - I'd love to see tests for returning false, throwing an error, and rejected promises.
  • It would be really cool if check operated on sync or async properties, returning a Promise in the case of async properties, though perhaps that could be saved for a future PR.
  • Test runner integrations here would be nice as well - let's make sure that async or (done) tests in appropriate runners do the right thing.
  • We'll want to make sure the flow and typescript type definitions (found in /type-definitions) are up to date with this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants