addon owned service with staticAddonTrees breaks DI

[void-mAlex]

Uses ember-scroll-modifiers
this relies on a service
https://github.com/elwayman02/ember-scroll-modifiers/blob/master/addon/services/observer-manager.js

which is NOT re-exported but only used in the modifier inside the addon itself

https://github.com/elwayman02/ember-scroll-modifiers/blob/master/addon/modifiers/did-intersect.js#L14

reproduction can be found here

• git clone https://github.com/void-mAlex/full_static_flags_reproduction.git
• cd full_static_flags_reproduction/static_embroider_v3
• pnpm install
• pnpm ember serve
• Visit your app at http://localhost:4200.

will see a js exception in the console and brokewn text on screen

to 'fix'
in ember-cli-build.js comment out
staticAddonTrees and staticAddonTestSupportTrees and restart ember server and it will show it works

core of the issue seems to be the mechanism by which DI system works

scroll modifiers exports a did-intersect modifier which when used in the host app gets an owner of the host app
when service injection looks up the requested service from inside the modifier it checks only on the owner where
due to staticAddonTrees we no longer register services from addons (re-exported or otherwise)

short of major enhancement of the DI system I am proposing we allow services to still be registered with staticAddonTrees enabled

any objections @ef4 ?
looking to expand on void-mAlex@ac1d96e

[ef4]

First, a rant:

This use case has never been documented as public API and it has no benefit over just doing the normal thing. Doing the unconventional thing here lets you make a one character change over the conventional thing:

-@service('ember-scroll-modifiers-observer-manager') observerManager;
+@service('ember-scroll-modifiers@observer-manager') observerManager;

Is that really worth being weird?

But second, yes, we should try to make it work since that is the whole point of embroider. Absorbing the weirdness so other people don't have to. 😅

Doing the obvious fix for this would make every app a little bigger, even though the vast majority of them do stick to the conventional thing and don't need this behavior.

I'd feel better about a fix for this if it was done as a staticServices flag that actually analyze all the injections, analogous to staticComponents. Components have a similar issue, because people can say {{component "some-addon@some-component"}}. But there it's mitigated by staticComponents: true because that can follow the reference, even with the @ in it.

If we did the same thing for service injections we could make @service('ember-scroll-modifiers@observer-manager') work without forcing every service in every addon to be eagerly registered just in case somebody wants to consume it this way.

[ef4]

To be clear, when I say "make every app a little bigger" I'm mostly referring to the cost of the extra registrations. The implementations of the services are already in there for sure right now.

Whereas a staticServices flag would actually let services get shaken out when unused and get split along with routes, which is not a thing we have today and would be a potentially large benefit for some apps.

[void-mAlex]

so I'm extra confused now
I've updated the reproduction repo with a few more examples

Doing the unconventional thing here lets you make a one character change over the conventional thing:

can you please go into a bit more detail on what is the conventional thing here?
is it to re-export the service?

from my testing it looks like unless you do you could only use the <my_addon_name>@<service_name> to inject it in the addon or any consuming app (even if not re-exported)
as far as I can tell that is the usage in the modifier where the service is invoked

I'm a bit surprised that if you don't have it re-exported you cannot use @service('service-name') within the addon to access it

for the solution are you saying when staticAddonTrees is true also read a new staticAddonServices? should that be different from app services?
this check happens in the v1 addon compat layer, for the analysis to be done isn't it too early? as we don't know who or how it would call it until we get to stage3
any hints on how we can do that analysis?

also opened the below pr so we can explore what I'm proposing as the solution I can see right now

[ef4]

is it to re-export the service?

Yes. If you ember generate service ... in an addon you get a re-exported service.

I'm a bit surprised that if you don't have it re-exported you cannot use @service('service-name') within the addon to access it.

This is consistent with how everything works in traditional ember. You also cannot invoke your own helpers or components or modifiers if they're not re-exported into the app. It would be weird for this one thing to be package-scoped when nothing else is.

for the solution are you saying when staticAddonTrees is true also read a new staticAddonServices?

No, I don't really want to complicated staticAddonTrees like this. I was OK with the special exception for engines because engines are kinda addons but also kinda apps, so it makes sense that staticAddonTrees doesn't perfectly apply to them. Also that didn't complicate the configuration options. But for regular addons with regular services in them, I don't want to start carving out exceptions.

Our max compatibility mode already supports this weird use case because staticAddonTrees defaults to false. I think if you want to leave max compatibility mode by setting staticAddonTrees: true you will need to stop doing this weird thing, or work around it.

I mentioned staticServices because that is one possible upgrade path: you could get your app working with staticServices enabled and then it would be safe for you to enable staticAddonTrees, because staticServices will have already inserted point-of-use import statements for even these addon-scoped injections. This is how things already work today for people who have usage like {{component "some-addon@thing"}} -- if they try to turn on staticAddonTrees they will have an error, but if they first get staticComponents working, then they can turn on staticAddonTrees.

Another workaround is to use a compat adapter for the offending addon that puts the service into implicit-modules explicitly:

// ember-cli-build.js
const { V1Addon, compatBuild } = require('@embroider/compat')
// ...
compatBuild(app, Webpack, {
  compatAdapters: new Map([
    ['ember-scroll-modifiers', class extends V1Addon {
      get packageMeta() {
        let meta = super.packageMeta;
        meta['implicit-modules'] = [...(meta['implicit-modules'] ?? []), './services/observer-manager.js'];
        return meta;
      }
    }]
  ])
});

this check happens in the v1 addon compat layer, for the analysis to be done isn't it too early? as we don't know who or how it would call it until we get to stage3

This is correct, I'm saying that staticAddonTrees would keep doing what it does in stage1. The service would not be put into implicit-modules automatically. But in stage3, during babel, when we look at did-intersect.js we would see @service('ember-scroll-modifiers@observer-manager') observerManager; and therefore insert something like:

import "#embroider_compat/services/ember-scroll-modifiers@observer-manager"

into that file, which we would handle with a new case here that generates the code necessary to import and window.define() the actual implementation.

[void-mAlex]

thanks for the quick feedback,
I get what I can do locally as a fix for this
As far as solutions for this to be a thing that just works I see the following

1. the open I have open now that introduces a new flag + any feedback I get for it
2. different pr that ships a compat adapter by default for that one package
3. a more comprehensive solution using the new stage3 babel parser to handle this edge case

it feels to me that the best thing to do would be item 3 but also the most work for it
I would like to hear your opinion on how I should proceed on this or if I missed an option from the list

[void-mAlex]

@mansona strictly speaking this is a bug as seen from a classic build perspective where something used to work and it no longer does
only if we choose to go down the path of option 3 is this an enhancement to embroider