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

Provide async variant of pushNewScope.init function. #258

Closed
bitsydarel opened this issue Feb 19, 2022 · 27 comments
Closed

Provide async variant of pushNewScope.init function. #258

bitsydarel opened this issue Feb 19, 2022 · 27 comments

Comments

@bitsydarel
Copy link

Currently pushNewScope.init is sync, it's inconvenient if you want for example to execute some async call in the init function before scopeChanged is called.

@escamoteur
Copy link
Collaborator

Wait, the async function should get executed before the new scope is pushed?
that's not how it is intended because the initfunction should allow you to register new instances the moment the new scope is created. you can register new async instances there too

@bitsydarel
Copy link
Author

Yes correct, the async function should be executed before scopedChanged event.

there’s cases when you want to have async code in the init function.

also provide a variant of init function that’s async does not break the intended use case.

@escamoteur
Copy link
Collaborator

but why not call that async code before you call the push? the init function is clearly executed after the push

@bitsydarel
Copy link
Author

Let me provide some code example.

Let say you have an application that has dependency modules that need to be initialized in order.

So one module need to complete initializing his dependency graph completely before the next one does.

_backend.pushNewScope(
      scopeName: scopeName,
      init: (GetIt di) {
        // we use for loop because the order of execution should match
        // the order in which the list was created.
        for (final DiModule module in modules) {
          module.initializeDependencyGraph(di); // return void
          module.waitForAsyncDependencies(di); // return Future<void>
        }
      },
    );

So module.initializeDependencyGraph is called directly as it's only registering types and return void.

Example:

void initializeDependencyGraph(GetIt di) {
    di
      ..registerSingletonAsync<AccountRemoteDataSource>(
        // creation code
        dependsOn: <Type>[TegNetworkManager],
      )
      ..registerSingletonAsync<AccountLocalDataSource>(
        // creation code
        dependsOn: <Type>[SharedPreferences],
      )
      ..registerSingletonAsync<AccountRepository>(
        // 
        dependsOn: <Type>[AccountLocalDataSource, AccountRemoteDataSource],
      )
      ..registerSingletonWithDependencies<GetCurrentAccountUseCase>(
        () {
          return GetCurrentAccountUseCase(
            di.get<GetCurrentMemberUseCase>(),
            di.get<AccountRepository>(),
          );
        },
        dependsOn: <Type>[AccountRepository],
      )
      ..registerSingletonWithDependencies<GetMemberAccountsUseCase>(
        () => GetMemberAccountsUseCase(di.get<AccountRepository>()),
        dependsOn: <Type>[AccountRepository],
      )
      ..registerSingletonWithDependencies<UpdateMemberCurrentAccountUseCase>(
        () {
          return UpdateMemberCurrentAccountUseCase(
            di.get<HostDeviceInfoRepository>(),
            di.get<MemberRepository>(),
            di.get<TegNetworkManager>(),
            di.get<GetCurrentMemberUseCase>(),
            di.get<GetCurrentAccountUseCase>(),
          );
        },
        dependsOn: <Type>[HostDeviceInfoRepository],
      );
  }

However each module can have async dependencies that initialization need to be awaited.

That why we have the waitForAsyncDependencies method.

Example:

  @override
  Future<void> waitForAsyncDependencies(GetIt di) {
    return Future.wait<void>(<Future<void>>[
      di.isReady<AccountLocalDataSource>(),
      di.isReady<AccountLocalDataSource>(),
      di.isReady<AccountRepository>()
    ]);
  }

Now the problem is, since the init callback of pushNewScope is not async.

We are not waiting for module.waitForAsyncDependencies to finish before moving to the next module initialization.

_backend.pushNewScope(
      scopeName: scopeName,
      init: (GetIt di) {
        // we use for loop because the order of execution should match
        // the order in which the list was created.
        for (final DiModule module in modules) {
          module.initializeDependencyGraph(di);
          // unawaited futurre, so no garrantee it will complete before the graph is initialized.
          module.waitForAsyncDependencies(di); 
      },
    );

The current workaround to this right now is to call waitForAsyncDependencies later.

_backend.pushNewScope(
      scopeName: scopeName,
      init: (GetIt di) {
        // we use for loop because the order of execution should match
        // the order in which the list was created.
        for (final TegModule module in modules) {
          module.initializeDependencyGraph(di);
        }
      },
    );

    // we use for loop because the order of execution should match
    // the order in which the list was created.
    for (final TegModule module in modules) {
      await module.waitForAsyncDependencies(_backend);
    }

This workaround is not ideal because what if we were using onScopeChanged to do execute some code when the new scope is pushed and all of his types were registered and initialized ?

Introducing async version of init would sold that.

@escamoteur
Copy link
Collaborator

Why aren't you using the dependsOn feature when registering your objects and async registration?

@bitsydarel
Copy link
Author

bitsydarel commented Feb 21, 2022

The new variant of pushNewScope would look like this.

Future<void> pushNewScope({
Future<void> Function(GetIt getIt)? init,
String? scopeName,
ScopeDisposeFunc? dispose,
}) async {
    // removed assert to simplify the code
    _scopes.add(_Scope(name: scopeName, disposeFunc: dispose));
    await init?.call(this);
    onScopeChanged?.call(true);
  }

With this variant my code would look like this.

_backend.pushNewScope(
      scopeName: scopeName,
      init: (GetIt di) async {
        // we use for loop because the order of execution should match
        // the order in which the list was created.
        for (final DiModule module in modules) {
          module.initializeDependencyGraph(di);
          // Now we're properly waiting for the module to complete his initialization.
          await module.waitForAsyncDependencies(di); 
      },
);

@bitsydarel
Copy link
Author

bitsydarel commented Feb 21, 2022

@escamoteur with dependsOn the problem is that every module has to know that the dependency it's depending on is async.

Imaginate a project with different packages/folders with their own dependency graph and many dev maintaining that those graph.

you will have to keep track of how dependency are initialized for each package/folder (features).

Or you will have to setup dependency graph for a giant app in a single file.

Which would make it a GOD file that everyone will be touching at every moment.

@escamoteur
Copy link
Collaborator

you don't need to use async to depend on a module. if you push a scope you are in total control what gets there initialized.

@bitsydarel
Copy link
Author

The ideal world would be:

Writing a feature that's dependent on some dependency developed by another feature.

You don't have to know out it's registered (async or not async).

You just write.

GetIt.get()

Also devs writing their features won't have to expose their facade implementation to other features or expose how they create those classes. Which would be required if you have single file that do DI.

@bitsydarel
Copy link
Author

@escamoteur If you depends on a class that was not registered with async, you will get a error from GetIt

@bitsydarel
Copy link
Author

'Dependent Type $dependency is not an async Singleton'

@escamoteur
Copy link
Collaborator

ok, but on the other side as non async instances are registered immediately you should to them first. if they depend on something you can use registerSingletonWithDependencies

@escamoteur
Copy link
Collaborator

OK, I think I misunderstood one thing. you don't want to execute the init before the scope is created because then your registrations would not get into the new scope

@escamoteur
Copy link
Collaborator

I could make a FutureOr for the init then we would not make a breaking change.
Still I hardly can imagine a constellation where the init features of get_it are not enough.

@bitsydarel
Copy link
Author

So basically if you want to say you depends on a type, you have to know that it's was registered async.

What i'm trying to do, is to abstract the knowledge of how upper dependency are created.

The downside with registerSingletonWithDependencies is

/// registers a type as Singleton by passing an factory function of that type
/// that will be called on each call of [get] on that type
/// [T] type to register

I'm using that for use case as i don't mind if they get recreated on each call of GetIt.get but for example for repository or data sources, i would like them to be unique in their scope

@bitsydarel
Copy link
Author

OK, I think I misunderstood one thing. you don't want to execute the init before the scope is created because then your registrations would not get into the new scope

That's correct.

@escamoteur
Copy link
Collaborator

wait? this is the comment for registerSingletonWithDependencies ?

that's false. it's not called at every call to get

@escamoteur
Copy link
Collaborator

I personally keep my main modules initilialzed in a god file so that I always know were to look.

@bitsydarel
Copy link
Author

bitsydarel commented Feb 21, 2022

I could make a FutureOr for the init then we would not make a breaking change.
Still I hardly can imagine a constellation where the init features of get_it are not enough.

Then you would still have to make the pushNewScope to return Future as you will have to await init before calling scopeChanged.

@bitsydarel
Copy link
Author

wait? this is the comment for registerSingletonWithDependencies ?

that's false. it's not called at every call to get

Yes.

Then the documentation of that method should be updated

@bitsydarel
Copy link
Author

bitsydarel commented Feb 21, 2022

I personally keep my main modules initilialzed in a god file so that I always know were to look.

That is fine for small application or app with not so much contribution.

Imaginate an app with 10+ big feature and all of those feature are developped by different teams for example.

Coming from native mobile development or backend that usual. But i guess flutter don't have much public big project like that.

We have a big application that is still growing and teams working on different part of that app and exposing public interfaces of services, repositories that can be used by other feature, without leaking implementation details.

If it's a single file all of those team/devs would be working on this file and updating it. this will create conflict on every PR or Merge.

@bitsydarel
Copy link
Author

bitsydarel commented Feb 21, 2022

@escamoteur I think the best option would be to just provide an async version of pushNewScope so it's won't be a breaking change.

Just a new possibility for those who need it.

@escamoteur
Copy link
Collaborator

OK, that's an idea

@escamoteur
Copy link
Collaborator

will look into it tomorrow. Are you also using the get_it_mixin?

@bitsydarel
Copy link
Author

will look into it tomorrow. Are you also using the get_it_mixin?

In some small part of the application, yes and other projects.

@escamoteur
Copy link
Collaborator

added in the latest version. I just forgot to include it in the changelog

@bitsydarel
Copy link
Author

Missed this comment! @escamoteur thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants