Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Feature/app endpoints #36

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nathanmalishev
Copy link

@nathanmalishev nathanmalishev commented Aug 10, 2020

Summary

This PR fixes/implements the following features

Added the following API requests to the sdk

Added two relevant tests & documentation.

@bmann bmann requested review from dholms and expede August 11, 2020 07:09
src/types.ts Outdated
@@ -16,3 +16,7 @@ export type Register = {
password: string
email: string
}

export type Apps = {
[property: string]: string
Copy link
Member

@expede expede Aug 11, 2020

Choose a reason for hiding this comment

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

🤔 Since this is mainly app names, have you considered something like AppName[]? We will likely have an App type in the future, so this may conflict with what one would expect from a plural of those.

Copy link
Member

Choose a reason for hiding this comment

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

That said, I’ll leave it to @icidasset and @dholms to weigh in here; they’re deeper in the TS client than I am (it’s been a while)

Choose a reason for hiding this comment

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

I would do something along the lines of:

type App = { domain: string }
type Apps = Array<App> // sorted by domain

Choose a reason for hiding this comment

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

And maybe remove the Apps type entirely, and just use Array<App> instead.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks guys for the review! I made the type changes, hopefully thats more along of the lines of what you wanted

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

Successfully merging this pull request may close these issues.

None yet

3 participants