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

Structure project to conform to DI with backwards compatibility. #36

Closed
wants to merge 3 commits into from

Conversation

acupofjose
Copy link
Collaborator

In reference to supabase-community/supabase-csharp#23

  • BREAKING CHANGE: Almost all method signatures now return Interfaces
  • Interfaces created for anything that was publicly accessible.
  • Additional methods added that allow for generic types accepting interfaces, allowing for injection of types but defaulting to the shipped, concrete classes.

@HunteRoi
Copy link

HunteRoi commented May 26, 2022

Hello there! Sorry for the late comeback.

I have taken a look at your PR. Here are my comments on it:

  • don't interface everything. Follow this simple rule: if there is some kind of logic (other than automated properties), then interface is required. Otherwise, use the class.
  • don't use generic methods directly in classes that implement an interfaces. Rather define generic methods in your interfaces and let the implementation only deal with specific behavior. Instead of
public interface IGotrueApi
{
    Task<T> CreateUser<T>(string jwt, IAdminUserAttributes attributes = null) where T : User;
     ....
}

public class Api : IGotrueApi
{
    public Task<T> CreateUser<T>(string jwt, IAdminUserAttributes attributes = null) where T : User
    {
        // code
    }
    
    public Task<User> CreateUser(string jwt, IAdminUserAttributes attributes = null) => CreateUser<User>(jwt, attributes);

     ....
}

go like this:

public interface IGotrueApi<TUser, TUserList, TSession>
    where TUser : User
    where TUserList : UserList
    where TSession : Session
{
    Task<TUser> CreateUser(string jwt, AdminUserAttributes attributes = null);
    ...
}

public class Api : IGotrueApi<User, UserList, Session>
{
    public Task<User> CreateUser(string jwt, AdminUserAttributes attributes = null)
    {
        // code
    }
    
     ....
}

Remember that the idea is to have an interface describing the actions you can do with any implementation of the interface. Maybe today your Api is an implementation of IGotrueApi<User, UserList, Session> but tomorrow you might wanna add support for an implementation of IGotrueApi<SpecificUser, SpecificUserList, SpecificSession>...

On a side node, please see the updated version of this PR here: https://github.com/HunteRoi/gotrue-csharp/tree/dev/structure-with-dependency-injection.

EDIT: this is a step-by-step procedure, we might wanna change things later. Let's find out what's the best implementation we can do to prevent breaking changes but also fit other needs.

@acupofjose
Copy link
Collaborator Author

Thanks for the feedback! I'll adjust and come back with some changes - that makes a lot more sense to me!

@HunteRoi
Copy link

Any news on this @acupofjose ?

@acupofjose
Copy link
Collaborator Author

@HunteRoi oop. To be honest, it's in exactly the same spot as I left it here. Thanks for the encouragement, needed a ping on it.

@acupofjose
Copy link
Collaborator Author

Closing in favor of #40 @HunteRoi

@acupofjose acupofjose closed this Oct 30, 2022
@acupofjose acupofjose deleted the dev/structure-with-dependency-injection branch October 30, 2022 03:45
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.

2 participants