[Question] Tree shaking services

[Windvis]

I recently created a PR for an addon which removed the app re-export for a service that's considered private API and replaced it with an injection string that uses the namespaced name (@service('ember-breadcrumb-trail@breadcrumbs') breadcrumbs).

This almost worked perfectly, but I noticed the CI job for embroider optimized failed. Looking more closely at it, it seems the service is not part of the final bundle so somewhere during the build process it gets marked as unneeded and dropped. I found this surprising since I explicitly inject it in multiple helpers.

I looked around at the documentation and specification but couldn't really find an answer to how the code analysis works. Does Embroider look at container lookups when analyzing the code?

While experimenting I noticed that explicitly importing and registering the service in the container does fix the problem (with the class import being the trigger I guess). Is this what's required to make services work that aren't re-exported for the app folder?

I don't think it's a major issue since not re-exporting a service seems like a niche use-case and most addons use a - prefixed service name for private services instead. I just wanted to verify that this is intended behavior.

[ef4]

Does Embroider look at container lookups when analyzing the code?

No, it doesn't. In general they aren't statically analyzable.

A lot of the common cases are analyzable, but that leaves behind some painful and surprising edge cases that we would either need to make into annoying build errors or just let apps break.

I'm open to adding a staticServices option so people can experiment with getting fully static analysis of services, but nobody has prioritized working on that at the moment.

While experimenting I noticed that explicitly importing and registering the service in the container does fix the problem

Yes, you can manage it directly like that and it will work.

Going forward, I hope people will help design improvements to Ember's API in this area to help making it easier for tooling (including both Embroider and TypeScript) to statically understand the dependency injection system).

[Windvis]

@ef4 Thanks for the clarification!

Going forward, I hope people will help design improvements to Ember's API in this area to help making it easier for tooling (including both Embroider and TypeScript) to statically understand the dependency injection system).

Isn't this something that RFC 585 tries to improve already? Or is that change alone not sufficient to make things properly analyzable?

One last question 😄. If staticServices isn't a thing at the moment, what exactly causes the service to be dropped in "optimized" mode but not in "safe" mode? And how would staticServices work differently?

[ef4]

One last question 😄. If staticServices isn't a thing at the moment, what exactly causes the service to be dropped in "optimized" mode but not in "safe" mode?

That would be staticAddonTrees.

With staticAddonTrees enabled, the whole addon folder of ember-breadcrumb-trail is not pushed into the build, it's only pulled in by import statements. The import statement in app/services/breadcrumbs.js is what caused addon/services/breadcrumbs.js to be included. Your PR deletes that file, which removes the only reason for it to be included.

This is typically safe to do, because the addon trees are almost always consumed by imports -- often reexports from the app tree, sometimes direct imports into the app. The only exception is the thing you tried here: giving an "@" to the runtime resolver.

[ef4]

Isn't this something that RFC 585 tries to improve already? Or is that change alone not sufficient to make things properly analyzable?

That is not enough. That API is defined in terms of runtime values, but doesn't say anything about limiting the syntax. For example, this is legal under RFC 585:

class Example2 {
  @service({ namespace: 'global', name: (() => Math.random() > 0.5 ? 'session' : 'other-session')() })
  session;
}

That example is deliberately perverse, but the point is that a lot of legal code needs to become illegal to guarantee analyzability. Rather than start imposing not-normal-javascript rules on top of what looks like normal javascript syntax, it would be preferable to redesign the API so you can't so easily say things that need to be forbidden.

[Windvis]

@ef4 Thank you, this has been very educational!