Open djrodgerspryor opened 3 years ago
That refactoring work was done by @ggriffiniii, a bit more than a year ago.
I definitely agree that more useful token storage is required and wonder why we moved away from the trait based model (which you mention and I remember).
So I have a few proposals that seem viable:
External(Box<ExternalStorage>)
mimicking the old model, but keeping the enum (where I'm not aware of all the up- and downsides of doing it that way). It would also keep API compatibility.TokenStorage
trait. Breaks API, but seems most generally useful (and I remember that originally the idea was that mostly every serious consumer of this library would write their own storage).Please, you and @ggriffiniii, let me know what you think.
It's been a while, but I believe the reason for going with the enum rather than the trait was because returning futures via a trait requires a heap allocation and we could avoid that with the enum. I did a quick (non-exhaustive) look at many of the public consumers of yup-oauth2 and none were using a custom trait impl so I proposed removing it until a use case arose.
Sounds like we have such a use case, and my vote is for Option 1 creating another variant of the enum for custom instances of a storage trait. Seems like the best of both worlds by providing the flexibility of a custom implementation while the built-in implementation don't pay the cost of a heap allocation on every call.
Thanks for the quick responses!
Option 3 will be difficult to adapt for my use case (the custom storage provider actually combines the MacOS keychain storage with some encryption using a key unlocked by 2-factor authentication, it's pretty specific to our use case and not suitable as a generic storage backend).
I've been playing around with a prototype of option 2, but I'll stash that and have a go at option 1 later today so that we can see how it looks.
I've got a working version of option 1 working here: https://github.com/dermesser/yup-oauth2/pull/146
Using the new interface to implement our custom storage type was pretty painless.
Let me know what you think about the approach!
I too need this!
Any movement on this?
I'm trying to migrate from v1 to the latest version. I have a custom storage backend for secure MacOS keychain storage written as an implementation of the old
yup_oauth2::TokenStorage
trait, but as far as I can tell, there's no way to do this in the new version, and none of the existing storage options suit my needs.Would you be open to replacing the current
Storage
enum with a trait instead?