-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add async methods to CRUD operations #31
feat: add async methods to CRUD operations #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jordanpowell88! Nice job!!
Looking through this a couple things to come to mind, what do you all think about wrapping the non-async functions and reusing them as opposed to redefining the existing logic?
So for example:
getAllAsync
would become:
public getAllAsync(): Observable<T[]> {
return of(this.getAll());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about typing the return type.
I echo @wesleygrimes comment as well. Helps to centralize the logic. |
Agreed on wrapping the methods. Don’t know why I didn’t think of that originally :facepalm |
It's ok, I would have done the same thing |
bug: add closing back tick to jscomment chore: add type for queryAsync method
…ll88/in-memory-db into feature-async-methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost, just a couple tweaks.
… expected record as an observable if given valid id
…ed given an invalid id
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@all-contributors add @jordanpowell88 for code, tests |
I've put up a pull request to add @jordanpowell88! 🎉 |
Closes #23