Milad-Akarie / injectable

Code Generator for get_it
MIT License
565 stars 143 forks source link

Bind.ToType on auto-generated concrete implementation #2

Closed SandroMaglione closed 4 years ago

SandroMaglione commented 4 years ago

I have an abstract class that extends ChopperService. The chopper package auto-generates the concrete implementation of the class. I am trying to bind the generated concrete implementation of the abstract class using injectable.

I have tried to solve it like this, since chopper adds to the concrete class the prefix _$:

import 'package:chopper/chopper.dart';
import 'package:injectable/injectable.dart';

part 'user_remote_data_source.chopper.dart';

@ChopperApi(baseUrl: '/user')
@Bind.toType(_$UserRemoteDataSource)
@injectable
abstract class UserRemoteDataSource extends ChopperService {
  static UserRemoteDataSource create([ChopperClient client]) =>
      _$UserRemoteDataSource(client);

  @Get()
  Future<Response<String>> getUser();
}

However, injectable does not recognize the generated class, it prints an error:

[UserRepository] depends on [UserRemoteDataSource] which is not injectable!

Did you forget to annotate the above classe(s) or their implementation with @injectable?

and generates the code using the abstract class instead:

..registerFactory<UserRepository>(() => UserRepository(
        userRemoteDataSource: getIt<UserRemoteDataSource>())

Is there a way to solve this issue directly using injectable?

Milad-Akarie commented 4 years ago

Hey @SandroMaglione, This's a tricky one! you can't bind to a private class as it can't be accessed in the configure function anyways. I'm working on handling named constructors maybe I should include static construct functions too. I'm guessing if you're to do it manually you would do the following:

getIt.registerFactory<MyService>(() => MyService.create(getIt());
SandroMaglione commented 4 years ago

Yes, you are right @Milad-Akarie.

I solved by manually adding the class to get it inside configure() as you suggested:

@injectableInit
void configure() {
  getIt
    ..registerLazySingleton<ChopperClient>(
      () => chopperClient,
    )
    ..registerFactory<UserRemoteDataSource>(
      () => UserRemoteDataSource.create(
        getIt(),
      ),
    );
  $initGetIt();
}

Thank you. Keep up the great work, I love your packages :thumbsup: 🔥

dalewking commented 4 years ago

I find the addition of Bind annotations to the abstract classes that have references to particular concrete implementations completely unacceptable. The abstract class should not be tied in any way whatsoever to the concrete implementations. It should know nothing of their existence. The abstract class may not even have access to such classes (e.g. they are defined in completely different libraries)

Milad-Akarie commented 4 years ago

Hey @dalewking, That's actually a valid point, I guess the best way to handle this is by some sort of a module where you can declare your bindings. What do you suggest?

dalewking commented 4 years ago

That is what systems like Dagger do. I would like to night have to annotate the actual classes at all and just create modules that define what gets injected.

saschaernst commented 4 years ago

And/Or make it possible to annotate the implemented class referencing the abstract class. So i.e. a mock implementation is completely unknown to the implemented type. Otherwise an independent registration of the types as suggested by @dalewking would prevent boilerplate code to use 3rd party libraries

Milad-Akarie commented 4 years ago

@saschaernst That's what I actually ended up doing. There's still a need for registration module for registering external types.

@registerModule 
abstract class Module {
@Bind(Service)
ServiceImpl get serviceImpl;
}

Im having second thoughts because, usually you would bind the abstract to its implementation not the other way around. What do you think?

saschaernst commented 4 years ago

@Milad-Akarie looks fine with me, as I don't have to inherit from 3rd party classes just to add an annotation and can even keep my custom classes completely framework agnostic, I am happy. The specific semantics within this registration module can be up for discussion, but are definitely an isolated problem which would not affect any other part of the code. If people like the annotations like they are now, there definitely should be an option to annotate the implementation with the abstract, so at least you don't have to touch the abstract for every new implementation again and again. With these little fixes, I will be more than happy to use this library as it reduces the boilerplate code significantly.

Milad-Akarie commented 4 years ago

@saschaernst v0.2.0 is published You can now annotate the implementation with @RegisterAs(abstractClass) and pass in your abstract type.

@RegisterAs(Service)
@injectable
class ServiceImpl extends Service {}
Milad-Akarie commented 4 years ago

@SandroMaglione you can now annotate your static create methods with @factoryMethod inside of your abstract classes and injectable will treat them as factory methods. in v0.2.0

@injectable
abstract class UserRemoteDataSource extends ChopperService {
 @factoryMethod
  static UserRemoteDataSource create([ChopperClient client]) =>
      _$UserRemoteDataSource(client); ...
Milad-Akarie commented 4 years ago

@dalewking take a look at the new @registerModule annotation, it might give you just what you need.

Milad-Akarie commented 4 years ago

Thank you all for your efforts and useful discussions. I'm closing this now.

dalewking commented 4 years ago

Doesn't satisfy my needs whatsoever. Dependency injection should be completely separate from the injection type (i.e. the abstract class in your example) and the implementation class. I should be able to put the abstract class in one library, the implementation in another library and they should have no references whatsoever to a third library that holds the dependency injection. Until we get to that point in my opinion you are not even doing dependency injection, which by definition means it is possible to choose AT RUNTIME what gets injected for a particular dependency.

saschaernst commented 4 years ago

@dalewking But doesn't registerModule do exactly that?

dalewking commented 4 years ago

Sorry, just saw the parts from today. If it works the way I think it does that would work.