-
Notifications
You must be signed in to change notification settings - Fork 251
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
V7: Improve type definitions #682
Conversation
b779587
to
bc09a07
Compare
01fbe01
to
00d5342
Compare
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.
What's the rationale for introducing nesting types under AbstractTypes
/ common
?
As a TS user if I want to look at the type signatures of the SDK I think I'd rather see the more concise:
export function notify(error: NotifiableError, onError?: OnErrorCallback): void
compared with:
export function notify(error: common.NotifiableError, onError?: common.OnErrorCallback): void
or
export function notify(error: AbstractTypes.NotifiableError, onError?: AbstractTypes.OnErrorCallback): void
I'm not sure AbstractTypes
or common
adds much to the signature.
00d5342
to
2f2331a
Compare
I was on the fence about the nesting anyway – I saw it as a good way to structure the miscellanaeous types that were not typically needed by the user but were exported just case. But I have just flattened and exported them all at the top level, which removes the nesting and the dubious name. |
The PR improves the types in various ways
request
,app
anddevice
more specific, and reuses their definitions