[FEATURE] Use service registry in owner.lookup

[ef4]

This enhances the type of owner.lookup so it respects the (already existing) service type registry. This means that owner.lookup('service:foo') will have the correct type as long as you do:

declare module '@ember/service' {
  interface Registry {
    'foo': MyFooService;
  }
}

I recognize that this Registry might not be the long-term plan, but as long as it's defined and supported by the service injection decorators it seems helpful to also support it in lookup.

[chriskrycho]

The utility of this is obvious, of course, so if we keep the registry I think we should do it. My hesitation is that we need to make two decisions here before stabilization:

• whether to support the string-based registries at all
• what, if any, changes to make to them given the opportunity we have to make some long-needed breaking changes (e.g., should they refer to the class instead of the instance type?)

Notably, at this point the only place where the registries are useful constructs is in lookup. Accordingly, I’d like to discuss with @dfreeman and @jamescdavis before we merge (and @dfreeman is on vacation this week).

[chriskrycho]

We chatted about this today and we definitely think we should add it, but we need to make a tweak to the registry design itself first: we want to go ahead and switch to using the class (not the instance) as the value in the type registry—as we do in Glint. This is a thing we've been thinking about doing for a while, and the migration into Ember for official types seems like a good time to do it.

[chriskrycho]

Given the overall change in direction, this will update to be like this:

Suggested change

	lookup<Name extends keyof Registry>(fullName: `service:${Name}`): Registry[Name];
	lookup<Name extends keyof Registry>(fullName: `service:${Name}`): InstanceType<Registry[Name]>;

We can also do the same (with a little work) for factoryFor and its FactoryManager return type: the factoryFor will end up able to return the right thing as well.

[chriskrycho]

I spent a little time poking at this tonight, and considering what we need to do in terms of the module graph to make sure this is safe for actually publishing separately: We need to not introduce a direct reference to an export from the service registry in the @ember/owner module: doing so will introduce a circularity which we are trying to avoid. I have a couple ideas on module structure which I think will do the trick, but it will require some further experimentation.

[chriskrycho]

All right, the way we actually want to do this is:

First, introduce a new module, @ember/service/-private/owner-ext.d.ts. (We could just do this inline if we were not using declare module, but we are using declare module for good reasons described in #20180!) Then, in that module, import the service registry and update the Owner type via declaration merging:

import type { Registry as ServiceRegistry } from '@ember/service';
import type { FullName } from '@ember/owner';

declare module '@ember/owner' {
  export default interface Owner {
    lookup<Name extends keyof ServiceRegistry & string>(
      fullName: FullName<'service', Name>
    ): InstanceType<ServiceRegistry[Name]>;

    factoryFor<Name extends keyof ServiceRegistry & string>(
      fullName: FullName<'service', Name>
    ): FactoryManager<InstanceType<ServiceRegistry[Name]>>;
  }
}

(This also shows making FullName a type utility, not just an alias for ${string}:${string}; I'm back and forth on the value of it but I think I like it because it gives us a single source of truth for the type literal representation.)

There is a version of this on the type-registries branch I just pushed up.

The open question I have after looking at this and thinking about a comment @dfreeman made in Discord yesterday is: Is there any value in having the service registry switch over to using the class instead? Right now, the answer sort of appears to be no, because we don’t actually ever reference an actual class… and we don’t have a good way to describe the static side of a class in TS interfaces anyway. If we eliminate that constraint and go back to working in terms of the instance type instead, the above would be:

import type { Registry as ServiceRegistry } from '@ember/service';
import type { FullName } from '@ember/owner';

declare module '@ember/owner' {
  export default interface Owner {
    lookup<Name extends keyof ServiceRegistry & string>(
      fullName: FullName<'service', Name>
    ): ServiceRegistry[Name];

    factoryFor<Name extends keyof ServiceRegistry & string>(
      fullName: FullName<'service', Name>
    ): FactoryManager<ServiceRegistry[Name]>;
  }
}

This definitely has the two big upsides of (a) not asking people to change their existing registries and (b) just being a lot more convenient. It has the downside of being basically impossible to evolve in the other direction without a type-level breaking change of some sort in the future. (It is not possible to go from an instance type to its class while preserving all info about the instance, in general.) Given the general direction we’re headed with injections and services, though, I don’t think that’s all that big of a risk, so I’m mildly inclined to keep it to just using the instance side of the type.

Thoughts, esp. from @dfreeman @jamescdavis?

[dfreeman]

we don’t have a good way to describe the static side of a class in TS interfaces

It's definitely a pain to author against interfaces for the static side of a class, but TS will preserve the type info that's there. If we have a service like this and register the constructor type rather than the instance type:

class MyService extends Service {
  static secret = Symbol('🤫');
}

declare module '@ember/service' {
  export interface Registry {
    'my-service': typeof MyService;
  }
}

Then we can type the class field on FactoryManager as Registry[K] and have the static members show up:

owner.factoryFor('service:my-service').class.secret; // symbol

Whether that's something we care about supporting? My leaning is no.

Given the general direction we’re headed with injections and services, though, I don’t think that’s all that big of a risk, so I’m mildly inclined to keep it to just using the instance side of the type.

I think that's totally reasonable.

The only reason in my mind to consider switching over to the constructor type would be for consistency with the Glint template registry (as well as suggestions I've seen of Data switching away from instance types in the future, though I have no idea if that's actually a concrete plan or not).

Ultimately, I don't feel super strongly on this one and am happy to see it ship either way.

[chriskrycho]

The other bit is: if we decide it is important to change, we can later (a) trivially codemod to it and (b) support migration paths that enable fallbacks. I'm 👍🏼 on it with that in view.