Skip to content

chore(ts): api.jsx to api.tsx #14148

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Aug 13, 2019
Merged

chore(ts): api.jsx to api.tsx #14148

merged 19 commits into from
Aug 13, 2019

Conversation

dashed
Copy link
Member

@dashed dashed commented Jul 25, 2019 •
edited
Loading

NOTES

  • unable to come up with proper types for this.hasProjectBeenRenamed()
  • unable to come up with a deterministic function signature for error callback.

TODO

  • rewrite api mocks to TS

@dashed dashed added the WIP label Jul 25, 2019
@dashed dashed self-assigned this Jul 25, 2019
dashed
dashed commented Jul 25, 2019
View reviewed changes
dashed
dashed commented Jul 25, 2019
View reviewed changes
dashed
dashed commented Jul 26, 2019
View reviewed changes
@dashed dashed force-pushed the typescript/api-client branch from a6d8429 to 85e12c0 Compare July 26, 2019 01:18
dashed
dashed commented Jul 26, 2019
View reviewed changes
dashed
dashed commented Jul 26, 2019
View reviewed changes
src/sentry/static/sentry/app/api.tsx Outdated
@@ -296,7 +359,7 @@ export class Client {
success: (data, ...args) => {
includeAllArgs ? resolve([data, ...args]) : resolve(data);
Copy link
Member Author

Choose a reason for hiding this comment

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

There may be a bug for when includeAllArgs is true here. 🤔

/cc @lynnagara @billyvg @markstory

Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see all the arguments being forwarded by default. Having to remember to set includeAllArgs and also unpack an array to get at the xhr object is more tedious than it needs to be.

Copy link
Member

Choose a reason for hiding this comment

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

yeah -- we'd have to update the requestPromise calls to always expect an array

dashed
dashed commented Jul 26, 2019
View reviewed changes
src/sentry/static/sentry/app/api.tsx
@@ -96,6 +143,7 @@ export class Client {
if (req && req.alive) {
// Check if API response is a 302 -- means project slug was renamed and user
// needs to be redirected
// @ts-ignore
if (this.hasProjectBeenRenamed(...args)) {
return;
Copy link
Member Author

@dashed dashed Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

this.hasProjectBeenRenamed() expects JQueryXHR as the input; but this cannot be guaranteed.

Introduced in #8799

@dashed dashed mentioned this pull request Jul 26, 2019
Merged
@dashed dashed force-pushed the typescript/api-client branch from 44ca1ab to 611cafe Compare July 26, 2019 05:57
markstory
markstory reviewed Jul 26, 2019
View reviewed changes
src/sentry/static/sentry/app/api.tsx
};

type QueryArgs =
| {
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a leading | here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to remove it; but prettier seems to be adding this leading |.

src/sentry/static/sentry/app/api.tsx Outdated
@@ -296,7 +359,7 @@ export class Client {
success: (data, ...args) => {
includeAllArgs ? resolve([data, ...args]) : resolve(data);
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see all the arguments being forwarded by default. Having to remember to set includeAllArgs and also unpack an array to get at the xhr object is more tedious than it needs to be.

@dashed dashed force-pushed the typescript/api-client branch 2 times, most recently from 7867c8f to 742abeb Compare July 29, 2019 22:35
dashed
dashed commented Jul 30, 2019
View reviewed changes
src/sentry/static/sentry/app/api.tsx
type RequestCallbacks = {
success?: (data: any, textStatus?: string, xhr?: JQueryXHR) => void;
complete?: (jqXHR: JQueryXHR, textStatus: string) => void;
error?: FunctionCallback;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unable to type this properly based on its usage from this file alone. So I'm leaving it as FunctionCallback.

When Sentry is significantly migrated to TS, we can revisit and refine this type.

Copy link
Member

Choose a reason for hiding this comment

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

Should we be marking things like this somehow?

dashed
dashed commented Jul 30, 2019
View reviewed changes
src/sentry/static/sentry/app/api.tsx
IncludeAllArgsType extends true
? [any, string | undefined, JQueryXHR | undefined]
: any
> {
Copy link
Member Author

Choose a reason for hiding this comment

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

@markstory I'm able to add a conditional type so that the right promise type is returned depending on if includeAllArgs is true.

@dashed dashed marked this pull request as ready for review July 30, 2019 00:41
@dashed dashed requested review from markstory, lynnagara and a team July 30, 2019 00:42
@dashed dashed removed the WIP label Jul 30, 2019
@dashed dashed force-pushed the typescript/api-client branch from 865c659 to 351ed3e Compare July 30, 2019 04:34
@billyvg
Copy link
Member

billyvg commented Aug 2, 2019

@dashed is this ready for a final review?

@dashed dashed added the WIP label Aug 3, 2019
@dashed
Copy link
Member Author

dashed commented Aug 3, 2019

@billyvg not really no. I'm going to take a few more passes on it and then squash my commits.

@dashed dashed force-pushed the typescript/api-client branch 2 times, most recently from 0636923 to b53ae0a Compare August 8, 2019 20:27
@dashed dashed removed the WIP label Aug 9, 2019
@dashed dashed requested review from billyvg and evanpurkhiser August 9, 2019 17:04
@dashed
Copy link
Member Author

dashed commented Aug 9, 2019

Looks like travis-ci is happy with this one now.

@billyvg this PR should be ready to go with the mocks typed.

@dashed dashed mentioned this pull request Aug 10, 2019
Merged
8 tasks
@dashed dashed force-pushed the typescript/api-client branch from 918da0b to 4ce9039 Compare August 12, 2019 19:52
lynnagara
lynnagara approved these changes Aug 13, 2019
View reviewed changes
Copy link
Member

@lynnagara lynnagara left a comment

Choose a reason for hiding this comment

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

🚢

@dashed dashed force-pushed the typescript/api-client branch from ee2656e to 7ed19f9 Compare August 13, 2019 05:44
@dashed dashed force-pushed the typescript/api-client branch from 7ecf76c to 09128c5 Compare August 13, 2019 14:44
@dashed dashed merged commit 9d13ff3 into master Aug 13, 2019
@dashed dashed deleted the typescript/api-client branch August 13, 2019 15:10
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lynnagara lynnagara lynnagara approved these changes

@markstory markstory Awaiting requested review from markstory

@billyvg billyvg Awaiting requested review from billyvg

@evanpurkhiser evanpurkhiser Awaiting requested review from evanpurkhiser

Assignees

@dashed dashed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dashed @billyvg @markstory @evanpurkhiser @lynnagara