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

Provide a user-settable callback for when tokens are updated #400

Open
PaulOlteanu opened this issue Apr 19, 2023 · 6 comments
Open

Provide a user-settable callback for when tokens are updated #400

PaulOlteanu opened this issue Apr 19, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@PaulOlteanu
Copy link

Is your feature request related to a problem? Please describe

I'm creating a web-app that stores users' tokens in a database. After performing a Spotify request, I don't have a way to know if the token was refreshed, and therefore have no way to store the updated tokens in my database.

Describe the solution you'd like

I'd like a config option on the clients where I could provide a callback function that could be called when the token is refreshed, so that I can save it to my database.

I feel that this solution is much more "library-like" than the current one that optionally saves the token to a file.

Describe alternatives you've considered

One alternative that I've considered (that I don't think is very good) is to set a field on the client when the token is changed, that can be unset by a user once they've handled the new token.

Additional context

I could try to look into implementing this if we think it's a good thing to add to rspotify

@PaulOlteanu PaulOlteanu added the enhancement New feature or request label Apr 19, 2023
@ramsayleung
Copy link
Owner

Good point!

provide a callback function that could be called when the token is refreshed

It's a great idea, and in order to keep compatibility and doesn't affect the existing user, the behavior of the default callback function should save the token into a file.

@PaulOlteanu
Copy link
Author

PaulOlteanu commented Apr 24, 2023

I think it may be better to have this be a breaking change rather than try to keep the previous behaviour of reading/writing the token.

To me it seem strange to have that kind of stuff happening from a library (and in general I personally feel that there's other things that don't really belong as part of a library such as the env-file feature - I think the end user should be doing that themselves if they need envfile support).

Obviously I don't want to start conributing by dictating what should and shouldn't be done, but I thought I'd just share my opinion. Perhaps this could/should be discussed on a separate issue

@PaulOlteanu
Copy link
Author

I've opened a draft PR in case anyone has some early feedback. I'm going to look into trying to use generics rather than trait objects to see how that impacts the ergonomics for the user. It would mean the clients would be generic over the functions the user provides as callbacks.

If anyone has opinions on either option (dyn Fn vs impl Fn) I'd love to hear them

@ramsayleung
Copy link
Owner

I think it may be better to have this be a breaking change rather than try to keep the previous behaviour of reading/writing the token.

To me it seem strange to have that kind of stuff happening from a library (and in general I personally feel that there's other things that don't really belong as part of a library such as the env-file feature - I think the end user should be doing that themselves if they need envfile support).

I agree with your point, it's better to be a breaking change rather than try to keep the previous behavior of reading/writing the token. But in order to minimize the impact of making a breaking change for user/developer, I prefer to provide a feature named token-file or other names, to provide a feature to user to keep everything unchanged, which is disabled by default.

@PaulOlteanu
Copy link
Author

Yeah, apologies for not finishing this up myself sooner.

Anyway I found that due to the way generics are handled we wouldn't be able to make the clients generic over the callback type because Rust would require the function to implement Debug even if we store it in some wrapper type.

I linked some tests I wrote in your PR, thanks for finishing this up 👍

Copy link

Message to comment on stale issues. If none provided, will not mark issues stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants