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

Refactor client API #61

Merged
merged 9 commits into from
Aug 4, 2024
Merged

Refactor client API #61

merged 9 commits into from
Aug 4, 2024

Conversation

seth2810
Copy link
Contributor

@seth2810 seth2810 commented Jul 27, 2024

  • simplified error handling including retries in case of network and axios package related errors
  • moved access token management into separate auth manager
  • shared same http client (manager) instance between all SpotifyAPI sub-managers

@seth2810 seth2810 self-assigned this Jul 27, 2024
}

this.config.accessToken = res.data.access_token;
if (retryAmount === 5) {
if (statusCode >= 500 && statusCode < 600) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just reverted back changes made in one of the recent refactoring 4d6aeb9#diff-7a66a64f88500ed3f05b25e049670886f374899bcf5bfcd1a850c19ebaeff100L86-L92 because they cause the token update to fail after the first 5xx response status

@seth2810 seth2810 requested a review from saa July 28, 2024 00:21
Comment on lines 32 to 36
constructor(config: SpotifyConfig) {
this.config = config;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know of multiple projects that are using the <SpotifyApi>.config to retrieve the configuration options and update the tokens through there, due to the removal of this line of code it's not possible anymore. I would advice to set it as public in the constructor directly, saves some lines and then it isn't a breaking change for other projects that are making use of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for sharing @stijnvdkolk, will improve the solution according to this fact. Maybe we have anything else details which may make sense for the library clients?

@seth2810 seth2810 requested a review from stijnvdkolk July 29, 2024 13:19
@seth2810 seth2810 changed the title WIP: Refactor client API Refactor client API Jul 29, 2024
}
await sleep(retryAfter * 1_000);

requestConfig.retryAttempt = 0;
Copy link
Contributor Author

@seth2810 seth2810 Jul 29, 2024

Choose a reason for hiding this comment

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

As previously we don't limit rate limit response retries, and reset counter in case when this status appeared while we did retries for other statuses

@seth2810 seth2810 requested review from taroninak and removed request for stijnvdkolk July 29, 2024 13:42
@seth2810 seth2810 requested review from stijnvdkolk July 30, 2024 05:17
@seth2810 seth2810 merged commit 916e3eb into main Aug 4, 2024
2 checks passed
@seth2810 seth2810 deleted the refactor-api branch August 4, 2024 12:43
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.

4 participants