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

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
}

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.

3 participants