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

Convert UserAPI to Typescript #21

Open
wants to merge 38 commits into
base: minoka-development
Choose a base branch
from

Conversation

kakizaki55
Copy link

@kakizaki55 kakizaki55 commented May 20, 2022

1. What this does:

  • Added interface for the Person to use as the user Type.
  • Each method has an argument type and return type.
  • Each Array is of type ReadonlyArray
  • had to make changes to line 23, to reassign the array so the Array stays unmutated.
  • added UserFormMeta interface to add user and update user arguments. (id is not optional)
  • updateUser now takes 2 arguments (id, UserFormMeta), Updated Tests accordingly.

2. Ticket:
Issue #none

3. Media:

UserFormMeta interface

Screen Shot 2022-05-24 at 10 01 20 AM

unit tests

Screen Shot 2022-05-23 at 2 59 32 PM

4. How to test:
pull down code run yarn jest --watch

@kakizaki55 kakizaki55 closed this May 23, 2022
@kakizaki55 kakizaki55 reopened this May 23, 2022
@kakizaki55 kakizaki55 changed the base branch from master to minoka-development May 23, 2022 22:01
src/index.ts Outdated
const targetUser = this.list.find(user => user.id === id)
if (!targetUser) {
throw new Error("There are no users found with that id.")
}
return targetUser
}

updateUserById(updatedUser) {
updateUserById(updatedUser: Person): Person {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatedUserById should accept:
a) a user ID for lookup
b) fields to be updated

lookup the TS Utility Partial, or just create a new object with fields that may be updated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rachoac should this be refactored to actually update the found Person object here, rather than accepting a whole, new Person? @kakizaki55 and I have discussed 1-1 in zoom, so maybe not necessary to practice that pattern here.

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.

3 participants