Skip to content

Ts and update - #1

Open
mariannefeng wants to merge 14 commits into
masterfrom
ts-and-update
Open

Ts and update#1
mariannefeng wants to merge 14 commits into
masterfrom
ts-and-update

Conversation

@mariannefeng

Copy link
Copy Markdown
Collaborator

No description provided.

@shankj3 shankj3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, thanks for all the beautiful documentation!

return normalizeUrl(
path.replace(beginningSlashes, ''),
{
stripProtocol: true,
stripHash: true,
removeQueryParameters: true,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just question -- why are we keeping in query params here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

removeQueryParameters doesn't take in a boolean, it takes in a string regex. This line wasn't doing anything, so I took it out.

Comment thread src/http/requests.ts
export const authorizePatch = getPhinClient(HTTPMethod.PATCH);

//we have to do this for delete because when delete returns 204 no content, phin client crashes
//waiting on PR https://github.com/ethanent/phin/pull/38

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

😢

Comment thread src/oauth/index.ts
throw new Error('Invalid token response');
}

// set token expiration to be 2 minutes before stated expiration

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good move 🦺

Comment thread src/oauth/index.ts

type AsyncFunction = () => Promise<String>

export function jwt(oauthKey: string): AsyncFunction {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if there aren't comments here, do you still get the tooltip highlights on IDEs that render the jsdoc? i do like when they do that

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

oh...no tooltip on hover yeah. I like it more too though! I'll move the comment I already wrote over the jwt/clientCredentials functions.

@mariannefeng
mariannefeng requested a review from twardcox May 15, 2020 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants