injectable icon indicating copy to clipboard operation
injectable copied to clipboard

Rambling thoughts on environment

Open dalewking opened this issue 3 years ago • 6 comments

You have documentation around how to assign bindings to environments but not about how that is handled in the generated function. I can tell you that because even after looking at the code I misunderstood what the env param did.

I actually wrote up the below in an issue to ask for changes to that behavior under a mistaken understanding of how it worked. Luckily, just before submitting it I realized it didn't work the way I thought it did. So you should at least explain it better in the documentation.

But I think there is still some possible improvement here and some cases not handled. Your example of multiple environments is an OR relationship. Adding dev and test annotations means dev OR test environment. But what if you do want to do multidimensional environments as I explained below and I want an AND relationship e.g. @android AND @dev. So perhaps you could enhance the generated function to accept an optional filter parameter (that is mutually exclusive with the string parameter) that is a function that accepts a list of strings (the environments on the binding) and returns a boolean to say whether to do that registration. That would allow for more complex environment handling without putting a great deal of burden on you.

The way you have environment support set up it is designed around a single dimensional set (dev, test, prod). What I am envisioning is multiple dimensions to environments. So in addition to that dimension I could see android vs. iOS as a dimension.

One way to accomplish this with no changes on your side is to take the cross product of the dimensions where I would end up with 6 different values (android-dev, android-test, android-prod, etc.) This works but if I want to define something only for Android I have to pass the list of all of those combinations that apply, though I could define those subsets all in one place so it is doable.

But stepping back the real issue is that when you call the generated code you can only pass a single environment and it always registers the ones that were not environment specific.

So I can see 2 ways to go here to make this simpler:

The ideal way would be to support passing a list of environments and in your helper you look for an intersection between the list instead of just contains. This could be done as a breaking change that anyone using the newly generated code has to pass it as a list instead of a single value, or you add a flag to the annotation to opt-in to the new behavior. Unfortunately, Dart does not support various I believe. You could make the parameter dynamic and allow either passing a String or a list of Strings.

Another alternative is to add an optional parameter to the generated function that controls whether you do the bindings that are not environment specific. The idea here would be for the case I described above you would call the function more than once:

$initGetIt();
$initGetIt(osEnvironment, false);
$initGetIt(buildEnvironment, false);

I would prefer the pass a list of environments approach over the multiple call approach

dalewking avatar Aug 01 '20 16:08 dalewking

So the tldr; version of that is:

  • Add an optional environment filter parameter to the generated function that takes a list of strings and returns bool saying whether to register it.
  • Document the way environment is handled in the generated function

dalewking avatar Aug 01 '20 16:08 dalewking

Went back and looked at the code and I wasn't mistaken about the way that the code currently works. It is only designed to be called once because it always registers the bindings that had no environment annotations so it is designed for a single dimension of environment.

But the suggestion I made of allowing a filter function to be passed is sufficient to handle other cases.

dalewking avatar Aug 01 '20 17:08 dalewking

I get the need for passing a list of envs instead of one approach and I'm totally on board, could you give an example on how the filter function would work? and btw Thanks for all the great ideas, you should push a couple PR's to be on the list lol

Milad-Akarie avatar Aug 03 '20 08:08 Milad-Akarie

Here is what I am working on for my case of a filter generator function (not tested yet):

const Environment android = Environment("android");
const Environment iOS = Environment("iOS");

const buildEnvironment = {dev, test, prod};
const platformEnvironment = {android, iOS};

EnvironmentFilter environmentFilter(Map<Set<Environment>, Environment> environments) {
  assert(environments.entries.every((entry) => entry.key.contains(entry.value)));

  return (Set<String> registerFor) =>
        environments.entries
          .where((entry) => entry.key.any((env) => registerFor.contains(env.name)))
          .every((entry) => registerFor.contains(entry.value.name));
}

Which you might use as $initGetIt(g, environmentFilter({buildEnvironment:test, platformEnvironment:android})

The basic logic is that for each entry in the map I:

  • Check if any of the environments on the binding are in the set of environments for that dimension, if not skip that dimension
  • Verify that list of environments on the binding include all the requested environment.

FYI, I tried to do some fancy things with subclassing Environment to create annotations that included the set of choices but the generator only works with the exact Environment class and not subclasses.

dalewking avatar Aug 03 '20 14:08 dalewking

@dalewking I ended up changing some things, EnvironmentFilter is now a base class so it's easier to make variant filters and to also hold current environment keys which I'm planning on injecting internally so users can retrieve them later on.

What do you think about this? also any naming suggestions are welcome.


// users can extend this class to make their own filters
abstract class EnvironmentFilter {
  final Set<String> environments;

  const EnvironmentFilter(this.environments) : assert(environments != null);
  
  bool canRegister(Set<String> depEnvironments);
  
}
// simple filter when you directly pass a filter func 
// without having to extend the base filter class
class SimpleEnvironmentFilter extends EnvironmentFilter {
  final EnvironmentFilterFunc filter;

  const SimpleEnvironmentFilter({this.filter, Set<String> environments = const {}}) 
      : super(environments);

  @override
  bool canRegister(Set<String> depEnvironments) => filter(depEnvironments);
}

// I'm planning to include some filters I think are common
class NoEnvOrContainsAll extends EnvironmentFilter {
 const NoEnvOrContainsAll(Set<String> environments) : super(environments);
@override
  bool canRegister(Set<String> depEnvironments) {
    return (depEnvironments.isEmpty) || depEnvironments.containsAll(environments);
  }
}
class NoEnvOrContains extends EnvironmentFilter {}
class NoEnvOrContainsAny extends EnvironmentFilter {}

Milad-Akarie avatar Aug 04 '20 19:08 Milad-Akarie

@dalewking @Milad-Akarie you can also use a combination of .env file variables and NoEnvOrContainsAny environmentFilter

DennisMuchiri avatar Oct 11 '22 10:10 DennisMuchiri