marcglasberg / async_redux

Flutter Package: A Redux version tailored for Flutter, which is easy to learn, to use, to test, and has no boilerplate. Allows for both sync and async reducers.
Other
230 stars 41 forks source link

Dependency management via Environment #107

Closed craigomac closed 3 years ago

craigomac commented 3 years ago

Thanks for the great package! I really love the separation of concerns I get from this package, and it has some clear advantages over traditional redux.

One thing I am missing is dependency management. When I set up a bunch of GetIt dependencies at app launch, and then fetch them from a shared instance or global variable during action reduction, it feels like there's a lost opportunity. If Store is the window from my action logic to the rest of the world, why can't it also encapsulate my dependencies?

My suggestion, inspired by the Swift Composable Architecture and this talk, would be to instantiate Store with an environment, to which Store would also be generically typed (becoming Store<State, Environment>). Store would simply retain it and pass it as an argument to reduce on ReduxActions, where it would be used to fetch dependencies. Environment could be any type the user wants, but one could also simply pass along an instance of GetIt as environment!

What do you think?

marcglasberg commented 3 years ago

What would be the advantage of passing GetIt in the store and then to the actions, in relation to accessing them directly in the actions?

craigomac commented 3 years ago

In the PR I now propose a simpler method where instead of passing environment to reduce, it's just available in the same way as state.

I chose GetIt as the simplest possible refactor, but that was a mistake =). The real advantage is that we can remove it completely!

The advantage of allowing Store to own your dependency container is that it makes Store the one place from which your app is configured, whether for tests or anything else. Your Store implementation itself is already a great container, being accessible "up the tree" from widgets.

So instead of:

  GetIt.I.registerSingleton<AudioService>(AudioServiceImpl());
  GetIt.I.registerSingleton<APIService>(APIServiceImpl());

  // in ReduxAction subclass:

  reduce() {
    GetIt.I.get<AudioService>().playAudio(...) // hope it's registered or I crash here!
  }

I would have:

  class AppEnvironment {
    final AudioService audioService;
    final APIService apiService;
    // ...
  }

  store = Store<AppState, AppEnvironment>(
    initialState: AppState.initial(),
    environment: AppEnvironment(
      audioService: AudioServiceImpl(),
       apiService: APIServiceImpl()
    )
  )

  // in ReduxAction subclass:

  reduce() {
    environment.audioService.playAudio(...)
  }

Some advantages of this are:

marcglasberg commented 3 years ago

Problem is, you are forcing everyone to use it that way. While I like the idea, you are forcing everyone to declare the AppEnvironment in Store<AppState, AppEnvironment>. Could you propose something which is not a breaking change?

marcglasberg commented 3 years ago

What if Store accepts an optional Object? env variable. This could be made available to the actions and to the VmFactory as getters. To avoid having to cast it, the app can define BaseAction and BaseVmFactory that cast it to AppEnvironment.

craigomac commented 3 years ago

I've tried a few approaches but can't seem to find anything "safe" that preserves the environment type passing to the action.

I've mocked up what I think you're suggesting here: something that works with very few changes, which would simply fail at runtime if the environment was not of the correct type.

Here's the commit on my fork: https://github.com/craigomac/async_redux/commit/39c4c91387d0039d4e5e766de1d118e3d759fb4f.

marcglasberg commented 3 years ago

I'll have a look. I also believe I have a suggestion based on your idea.

marcglasberg commented 3 years ago

@craigomac Please see version [12.0.1] (in GitHub, not pub.dev), and run main_environment.dart.

Please tell me what you think.

craigomac commented 3 years ago

Looks good to me. I guess I still would lean towards a completely typed solution (being a Swift developer at heart!), but to be pragmatic in not breaking existing usages also makes sense.

marcglasberg commented 3 years ago

Ok. I'll add this to the readme file, and list your name in the "thanks" section so that your contribution doesn't get lost. I'll let you know when that happens. Thank you.

marcglasberg commented 3 years ago

@craigomac

Added a Dependency Injection section in the readme:

https://github.com/marcglasberg/async_redux#dependency-injection

Also there: The dependency injection idea was contributed by Craig McMahon.

Thanks!

craigomac commented 3 years ago

Cheers! Thanks for the great library!