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

Tree Shaking Capability #181

Closed
JAAvander opened this issue Mar 4, 2021 · 8 comments · May be fixed by #455
Closed

Tree Shaking Capability #181

JAAvander opened this issue Mar 4, 2021 · 8 comments · May be fixed by #455

Comments

@JAAvander
Copy link

I am building a fairly large project, using injectable for dependency injection. Currently I think injectable is the best dependency injection package for dart.
In this project, we at some point made the assumption that darts tree shaking would take care of taking out parts of code that would not be used in the production environment.
A while ago I was searching through issues on here, and found nothing about the topic, so I was suddenly uncertain of this assumption, and yesterday I took the time to validate it.

I set up a small project, plugged in some relatively large dependencies, as well as injectable and get_it. After that, I built some apks while running a code-size-analysis under the following conditions:

  • large dependencies not registered via injectable v, and unused
  • large dependencies not registered via injectable , but used
  • registered, but only in a different environment
  • registered, and in the active environment

Aside from the first one, all of them had the same analysis result. The first was clearly smaller then the rest. Since it was a small project, this is hardly a problem, but on a larger scale a lot of technically dead code would be shipped.

I presume that the reason for why dart can not tree shake the dependencies out when they are in a different environment is the complexity of the environment filter. and GetItHelper.
Maybe there is a simple solution that I have not thought of yet, where a custom implementation of the environment filter makes tree shaking possible again.

Are there any prior considerations about tree shaking that went into injectable?

@Milad-Akarie
Copy link
Owner

Milad-Akarie commented Mar 4, 2021

@JAAvander To be honest there aren't any prior considerations of any kind, I'll have to do something similar to your scenario in my day job real soon and I still have no idea where to start.

it seems like you've already put some work into this so maybe you can help add Tree Shaking support to injectable?
if you're not big on doing PR's you could just share your work or notes as comments right here.

@JAAvander
Copy link
Author

It would take some time until I could finish a proper pull request. A lot of work is piling up for me currently.
Regarding sharing my notes:

While I've known the topic for a long time, I'm not an expert on the topic of tree shaking in dart.
From what I could gather, there is a lot of unclarity of how well the dart compiler can figure out what can be tree shaken out.
There is a open issue on dart about this unclarity in dart documentation.

On the risk of going into too much detail, here is what I'm aware of:

Tree shaking summed up ... In generell, if something is not imported, it clearly would not be included. Furthermore, even if a library is imported, if classes and/or constants within it are not directly depended on, their code does not need to be included. It then comes down to how the compiler can tell that something is not being depended on. Say we are depending on some large library, and we have something like this:
if(constantBool){
  // Code that uses the library
} else {
  // Code that does not use the library
}

then given that the value of constantBool is known, the compiler is perfectly able to tree shake the library out. As soon though, as the bool can have a different value, the compiler has to include the library for the eventuality of it being used.
Of course not only constant bool values can be used. If there is a comparison that can be done at compile time, it would suffice to facilitate tree shaking.
For instance

if( constantString == "some string") ...
if( constantInt == 5 ) ...

would make the code tree shakable as well. Comparisons between all primitives can be resolved at compile time. For classes and functions it depends on the exact situation. As soon as classes and functions are involved, the aforementioned unclarity can be seen. Some functions can be resolved at compile time, given that all their arguments are consts, even though function calls can never count as const expressions.
As for classes, unused methods can be shaken out, but even for const classes, not all methods will resolve to a value at compile time.

Looking at injectable specifically, I think the problem that prevents tree shaking from happening is the use of sets to register things for specific environments.
I have run a few tests, and from what I could find, no operations on the standard sets could resolve at compile time. One would imagine this would be possible, at least for .contains, but apparently not.

I have tried to see to what extend I would have to modify the generated code, such that the app that is the result of the compilation has dependencies from unused environments removed. This is just an example, assuming that only an environment string is given, and only the default behavior is desired.
The code in this case would have to be changed from this:

const String _prod = 'prod';
const String _dev = 'dev';
const String _test = 'test';

GetIt $initGetIt(GetIt get,
    {String environment, EnvironmentFilter environmentFilter}) {
  final gh = GetItHelper(get, environment, environmentFilter);
  gh.factory<IChatService>(() => NetworkChatService(),
      registerFor: {_prod, _test});
  gh.factory<IChatService>(() => TestChatService(),
      registerFor: {_dev});
  return get;
}

to

const String _prod = 'prod';
const String _dev = 'dev';
const String _test = 'test';

GetIt $initGetIt(GetIt get,
    {String environment, EnvironmentFilter environmentFilter}) {
  final gh = GetItHelper(get, environment, environmentFilter);
  if (environment == _prod ||
      environment == _test)
    gh.factory<IChatService>(() => NetworkChatService(),
        registerFor: {_prod, _test});
  if (_dev == environment)
    gh.factory<IChatService>(() => TestChatService(),
        registerFor: {_dev});
  return get;
}

Of course this only imitates one specific filter. I'm still thinking of solutions, but it might not be possible to have the full power of custom filters, and allow tree shaking at the same time.

@JAAvander
Copy link
Author

Ok, I've been thinking, and here are some ideas for enabling tree shaking:

  • We simplify how services are registered, by using simple if-based checks. This is probably the worst option, since it would mean that complex filter functions as they are a feature right now would not continue working.
  • We add a separate flag that can be set in the @InjectableInit annotation, such as treeShakable. This would enable the above behavior and disable complex filters. That way it can be specifically enabled for users that need it, without limiting features for most users.
  • We handle what is injected at build time.
    Instead of giving a filter or environment to the injection init function we would give it to the @InjectableInit annotation. This way working with sets is still possible, theoretically enabling all existing features, while guaranteeing tree shaking to take place.
    These parameters to @InjectableInit can also be optional, such that the regular behavior could be preserved.

@mrverdant13
Copy link

I also consider filters usage is the critical factor here.

I would like to add another alternative: generating separated initGetIt functions per each environment.

This is related with @JAAvander's third idea, adding more config resolution regarding initialization setup.

@windrunner414
Copy link

I also think enable tree shaking is important and here is my issue: #191
I think it's the same issue

@leovinsen
Copy link

Hi everyone, did anyone manage to come up with a solution?

If not, any pointers on where to look at is welcome, and perhaps I could write a PR for it.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions

@anggaaryas
Copy link

btw, can we choose specific environment at build runner phase? not in runtime. maybe it correlate with tree shaking

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

Successfully merging a pull request may close this issue.

6 participants