-
Notifications
You must be signed in to change notification settings - Fork 11
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
Dependencies need updating and/or Change Request #192
Comments
Some changes to your library that you might want to consider:
By implementing No. 2 and No. 3, a developer could more easily build an implementation that does the first oauth2 steps, without having to reimplement code from The implementation of No. 3 would be something like this: private async tokenExchange(code: StravaCode) {
const payload = {
code: code,
client_id: this.config.clientId,
client_secret: this.config.clientSecret,
grant_type: 'authorization_code',
};
const params = {
method: 'post',
body: JSON.stringify(payload),
headers: { 'Content-Type': 'application/json' },
};
return fetch(STRAVA_URL.token, params)
.then((resp) => {
// resp is a TokenExchangeResponse object
});
}
export type TokenExchangeResponse = {
token_type: string;
expires_at: EpochSeconds;
expires_in: Seconds;
refresh_token: StravaRefreshToken;
access_token: StravaAccessToken;
athlete: SummaryAthlete;
}; (in the above I have declared The implementation of No., 2 would be something like this: public getAuthorizationUrl(options: AuthorizationUrlOpts = {}): string {
if (!this.config.clientId) {
throw new Error('A client ID is required.');
}
const opts = Object.assign(defaultAuthOpts, options);
return (
`${STRAVA_URL.authorize}?client_id=${this.config.clientId}` +
`&redirect_uri=${encodeURIComponent(opts.redirectUri)}` +
`&scope=${opts.scope}` +
`&state=${opts.state}` +
`&approval_prompt=${opts.approvalPrompt}` +
`&response_type=code`
);
} A final note: I am impressed with your clean implementation. I had implemented my own library a number of years ago, but have replaced my lower level API calls with your library because yours is better. My application has layers of code on top to support the full oauth2 lifecycle from a command line application, to hide |
Hey @jpravetz, thanks for this. I agree with you, this library could use some clean up and updates. Would you mind opening a PR with your suggestions? |
Not a problem and I will do so, so long as I have less trouble with the build this time. |
@jpravetz I can try to help with the build process once you open the PR. |
I believe your dependencies could use an update, and I am requesting your input on this. I forked and tried to update myself, but you're using a few extra tools I haven't used before, like
tsdx
, and have a more complicatedpackage.json
that I am used to.There are no notes in your README about building and testing, so maybe I am making mistakes.
Doing
npm install
as a first step resulted in lots of warnings, which is why I was going down the dependency update path. However all unit tests did pass, despite the warnings (but then they broke after my failed dependency update attempts).My intent was to potentially make a few minor changes to your library for a pull request, which is why I was try to build locally.
The text was updated successfully, but these errors were encountered: