-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Typescript typings #1134
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
fix: Typescript typings #1134
Conversation
typescript/raven.d.ts
Outdated
@@ -264,7 +264,7 @@ declare module Raven { | |||
sentry_key: string; | |||
}; | |||
onSuccess: () => void; | |||
onFailure: () => void; | |||
onError: (error: Error & { request: XMLHttpRequest }) => void; |
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.
I don't know TS syntax much, so is this request
already optional by using &
, or does it have to be annotated somehow? Because it can be omitted in the code.
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.
It's not, that'd have to be request?:
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.
@kamilogorek @SimonSchick I actually went through the code and thought that request is always set, but if you think it should be optional, it's fine with me - adjusted
typescript/raven.d.ts
Outdated
@@ -264,7 +264,7 @@ declare module Raven { | |||
sentry_key: string; | |||
}; | |||
onSuccess: () => void; | |||
onFailure: () => void; | |||
onError: (error: Error & { request: XMLHttpRequest }) => void; |
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.
It's not, that'd have to be request?:
typescript/raven.d.ts
Outdated
@@ -264,7 +266,7 @@ declare module Raven { | |||
sentry_key: string; | |||
}; | |||
onSuccess: () => void; |
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.
Btw: onSuccess(): void
in generally shorter and preferred for interfaces.
@SimonSchick @kamilogorek thx for the review, anything else I'm missing? |
Looks good to me. Thanks! |
Before submitting a pull request, please verify the following:
I tried applying the hack described here - #339 (comment) - in my angular (typescript) application, when I noticed that some types don't match.
I went through the implementation and hope that I've renamed / adjusted everything correctly. Would be cool if you could merge and release these fixes soon. Thx