-
Notifications
You must be signed in to change notification settings - Fork 26
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
Suggestion: Factor out leaky abstractions of oauth2_client #193
Comments
To be fair, point 1 is probably something to be raised with the oauth2_client project. |
Ok raised cmd-johnson/deno-oauth2-client#38 for point 1, but I still uphold concern regarding the other points even if this change is accepted in the oauth2_client lib. |
A dedicated kv_oauth config type protects against adoption of new features too, such as OpenID Connect support (#194) ... the API introduces a new interface for that config... https://github.com/cmd-johnson/deno-oauth2-client/blob/feature/oidc/src/oidc/oidc_client.ts#L6 This would difficult if A dedicated kv_oauth config type would be able to seamlessly add in the additional optional props for OIDC though, and the internal client creation could choose use the OIDCClient, if and only if the required additional props had been supplied. |
I agree that the 3 points made are indeed sub-optimal. However, I wouldn't say they are issues in this module. The difference between
We can use |
I've created cmd-johnson/deno-oauth2-client#41 to suggest making the |
kv_oauth sits atop oauth2_client, but exposes much of the inner workings of oauth2_client, this I feel is a leaky abstraction in that it leaves kv_oauth at risk from major oauth2_client API changes, and prevents (however unlikely) future consideration of alternatives, or even direct inclusion of just the essential parts of that lib should it ever become unmaintained.
One way to address this is to pass config rather than client as addressed by #174, but it's been discussed that the config adopts OAuth2ClientConfig directly.
There are three issues I have with this interface:
defaults.requestOptions
&defaults.stateValidator
defaults.scope
which feels unnecessarily nestedMy proposal is for kv_oauth to expose it own config type that better fits it use case and developer experience, exposing only options that are necessary for kv_oauth, and to protect it from potential future changes, abandonment, or even the consideration of alternatives to the underlying oauth2_client API.
I feel that this is something that could be neatly addressed now whilst in beta stage, as it'll be a lot harder to plug the leaks later.
The text was updated successfully, but these errors were encountered: