Skip to content
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

Open
jpravetz opened this issue Oct 6, 2023 · 4 comments
Open

Dependencies need updating and/or Change Request #192

jpravetz opened this issue Oct 6, 2023 · 4 comments

Comments

@jpravetz
Copy link
Contributor

jpravetz commented Oct 6, 2023

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 complicated package.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.

@jpravetz
Copy link
Contributor Author

jpravetz commented Oct 8, 2023

Some changes to your library that you might want to consider:

  1. export the various request object types from your resources folder. For example, GetActivityByIdRequest.
  2. add a method to build and retrieve the authorization URL from request.ts.
  3. add a method to do a token exchange using an oauth2 code to retrieve a RefreshTokenResponse.

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 request.ts.

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 type EpochSeconds = number, just for my own convenience, and am not suggesting you do this)

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 page and per_page limitations, and build context.

@jpravetz jpravetz changed the title Dependencies need updating? Dependencies need updating and/or Change Request Oct 8, 2023
@rfoel
Copy link
Owner

rfoel commented Oct 9, 2023

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?

@jpravetz
Copy link
Contributor Author

jpravetz commented Oct 9, 2023

Not a problem and I will do so, so long as I have less trouble with the build this time.

@rfoel
Copy link
Owner

rfoel commented Oct 9, 2023

@jpravetz I can try to help with the build process once you open the PR.

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

No branches or pull requests

2 participants