bitwarden / bitwarden/passwordless-dotnet
Deprecate Autotelic Abstraction
- Dominant language
- C#
- Stars
- 52
- Forks
- 17
- PR merge metrics
- No merged PRs in 30d
Description
Oleksii [called me out hard](https://twitter.com/Tyrrrz/status/1699376913226068153) 😄 for having an interface that will lead to unnecessary breaking changes. While I doubt many, if any, will create their own implementation of the `IPasswordlessClient` it is technically a breaking change to add a method to a public interface while it is not to add a new method to `PasswordlessClient`.
While I do like having interfaces for the purposes of mocking, the better more resilient thing to do is to make your own interface that you own in your application that wraps the usage you have of any said library. Bitwarden server follows this practice for our uses of Stripe with [`StripeAdapter`](https://github.com/bitwarden/server/blob/4b482f0a34b3811131fb4ab571267e11bd7b4de8/src/Core/Services/Implementations/StripeAdapter.cs). We should recommend users of our library, who want to mock our service, do the same.
What we can do today:
- Change examples and documentation that use `IPasswordlessClient` to `PasswordlessClient`
- Stop adding new methods to `IPasswordlessClient` and only add the implementation in `PasswordlessClient`.
When we do _other_ and only when we do other breaking changes we _can_ but don't have to:
- Make `IPasswordlessClient` have the `[EditorBrowsable(Never)]` attribute.
- We could do this before a breaking change without actually breaking anyone but users who had previous uses of `IPasswordlessClient` and want to use it elsewhere could be thrown off.
- Change the `AddHttpClient` method use `PasswordlessClient` as service instead of `IPasswordlessClient`
- This changes logs from looking like: `System.Net.Http.HttpClient.IPasswordlessClient.ClientHandler` to `System.Net.Http.HttpClient.PasswordlessClient.ClientHandler` this is breaking because some users could use that category name to tune logs. Rare, but could cause some annoyance to consumers.
- This doesn't need to be done other than people being slightly confused why the interface name is showing up in the logs when they don't use the interface at all.
- This also changes the name of the client for if people are manually configuring/injecting the `HttpClient` themselves, this is not a situation we support or document anywhere but it's something that can be done before that would no longer work.
- Write a recommended migration path of just removing the `I` from the type.
- (NOT RECOMMENDED) we could also take a more aggressive approach and add `[Obsolete]` to the type but I don't think this is warranted.
Contributor guide
Research direction
Start by locating the examples and documentation that use IPasswordlessClient, then review how PasswordlessClient and AddHttpClient are exposed. Done means updating the documented usage and determining how the interface should be handled without introducing the optional breaking changes described in the issue.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- api, developer-experience
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100