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

Update iff predicates typings to be either sync or async #509

Merged
merged 3 commits into from
Feb 18, 2019

Conversation

NickBolles
Copy link
Contributor

@NickBolles NickBolles commented Feb 14, 2019

Summary

  • Tell us about the problem your pull request is solving.
    The iff* hooks were typed to only accept synchronous predicate functions
    This PR adds
  1. Typescript type definitions for Asynchronous (in addition to the previous synchronous), Predicate functions
  2. Typescript type tests (in types/tests.ts) to test for Async Predicates
  3. 2 more tests for Iffelse to get it to 100% code coverage in normal (non-typescript) tests
  • Are there any open issues that are related to this?
    No
  • Is this PR dependent on PRs in other repos?
    no

If so, please mention them to keep the conversations linked together.

Other Information

There are a few small updates to docs to happen, there are more after my next PR

iffElse

arguments - {Function} predicate -> {Boolean | Promise | Function} predicate

iffElse

arguments - {Function | Boolean} predicate -> {Boolean | Promise | Function} predicate

@eddyystop
Copy link
Collaborator

eddyystop commented Feb 14, 2019

The predicate can already be a function which returns a Promise. Hence the function can be asynchronous, even an async function. I'm not not clear on what feature this PR adds.

@NickBolles
Copy link
Contributor Author

@eddyystop I think you miss-read the title. This is for the typescript types. They were previously restricted to synchronous predicate functions, which is incorrect.

I added the typescript types for both Sync or Async Predicate functions, as well as Typescript type tests for IFF, and a normal test to make test coverage 100%. (I'll update the OP with this info)

@eddyystop eddyystop merged commit 7108df5 into feathersjs-ecosystem:master Feb 18, 2019
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.

2 participants