From 8981b44ba518ef47315b5a37b1e5366346a42923 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Fri, 14 Jun 2019 21:52:16 -0400 Subject: [PATCH 01/14] wrote it --- text/0000-explicit-dependency-injection.md | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 text/0000-explicit-dependency-injection.md diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md new file mode 100644 index 0000000000..2cb509d648 --- /dev/null +++ b/text/0000-explicit-dependency-injection.md @@ -0,0 +1,75 @@ +- Start Date: 2019-06-14 +- Relevant Team(s): (fill this in with the [team(s)](README.md#relevant-teams) to which this RFC applies) +- RFC PR: (after opening the RFC PR, update this with a link to it and update the file name) +- Tracking: (leave this empty) + +# Explicit Dependency Injection + +## Summary + +When learning Ember and/or Dependency Injection, the question comes about how magic the injection strings are. The goal of this RFC is to propose an alternate default for injections that allow ctrl+clickability / "go to definition" from the injection to the class that defines the type of the injected instance. + +Note: while this RFC mainly talks in terms in services, this applies to all injections. + +## Motivation + +The main goal is to _use the platform_ and enable "go to definition" support from service definitions so developers can more easily discover the where and how their service is defined. + +## Detailed design + +instead of: +```ts +@service notifications; +@service('notifications') notifications; +@service('messages/dispatcher') dispatcher; +``` +This will be the new default: +```ts +@service(NotificationsService) notifications; +@service(MessageDispatcherService) dispatcher; +``` + +This means that the shorthand syntax of using `@service` without a parameter should be discouraged, and the use of the service class should be used in its place. + +At present, the service decorator wraps around the `ApplicationInstance#lookup` method -- something like this (roughly / hand-waiving the implementation details of decorators and getting access to the app instance): + +```ts +function service(name) { + return appInstance.lookup(`service:${name}`); +} +``` + +Instead, with this RFC, the service pseudo-function should check for the type of the parameter, and: + +```ts +function service(nameOrClass) { + if (typeof nameOrClass === 'string') { + return appInstance.lookup(`service:${name}`); + } + + return appInstance.lookup(nameOrClass); +} +``` + +In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. + +A new map can exist on the registry that can appropriately registers, unregisters, etc by class definition at the same time as the current registry behaves, allowing for backward compatibility with todays injection usage. + + +## How we teach this + +Instead of using strings or inferred injections, the guides should be updated to use the Class definition of a service that it intends to inject. + +Eventually, we'll want to introduce a deprecation for string injections to minimize different injection techniques. + +## Drawbacks + +- more verbose + +## Alternatives + + - Convert the dependency injection lookup to a `WeakMap` + + This would likely result in faster lookup, but would require more upfront work to change how the lookup method works on the `ApplicationInstance` / "owner". This _could_ be a separate RFC, but without benchmarking it's hard to say if this massive of a change would be worth it. + + - in C# + asp.net core, dependency injections are resolved in the constructor of a class. This would be _even more verbose_ than what is being proposed in this RFC, as for many situations in Ember, the constructor can be omitted. \ No newline at end of file From b20e512747912efb1f39a381be9fb8c05ffd5de1 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Fri, 14 Jun 2019 21:59:42 -0400 Subject: [PATCH 02/14] add question --- text/0000-explicit-dependency-injection.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 2cb509d648..c9e4eb4e97 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -72,4 +72,17 @@ Eventually, we'll want to introduce a deprecation for string injections to minim This would likely result in faster lookup, but would require more upfront work to change how the lookup method works on the `ApplicationInstance` / "owner". This _could_ be a separate RFC, but without benchmarking it's hard to say if this massive of a change would be worth it. - - in C# + asp.net core, dependency injections are resolved in the constructor of a class. This would be _even more verbose_ than what is being proposed in this RFC, as for many situations in Ember, the constructor can be omitted. \ No newline at end of file + - in C# + asp.net core, dependency injections are resolved in the constructor of a class. This would be _even more verbose_ than what is being proposed in this RFC, as for many situations in Ember, the constructor can be omitted. + + + ## Unresolved Questions + + These may be more for implementation, but as I was tracing how injections work.. it looks like the control flow path is: + - `@service` + - `appInstance.lookup` + - `engineInstance.lookup` (Application inherits from Engine) + - registry defined on `RegistryProxyMixin` + - instantiated via `this.buildRegistry();` + - registry is an instance of `Registry` + - defined at `ember.js/packages/@ember/-internals/container/lib/registry.ts` + - in here is where the bulk of the implementation of this RFC would live? (along with updating the type signatures of the methods all the way up the call tree (where not inferred)) \ No newline at end of file From ab8419e0c702911c887d255c759cd3b4d17aa6e5 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Fri, 14 Jun 2019 22:00:28 -0400 Subject: [PATCH 03/14] add pr link --- text/0000-explicit-dependency-injection.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index c9e4eb4e97..64e673e311 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -1,6 +1,6 @@ - Start Date: 2019-06-14 - Relevant Team(s): (fill this in with the [team(s)](README.md#relevant-teams) to which this RFC applies) -- RFC PR: (after opening the RFC PR, update this with a link to it and update the file name) +- RFC PR: https://github.com/emberjs/rfcs/pull/502 - Tracking: (leave this empty) # Explicit Dependency Injection From dbd520ad13154bfc4e6241265b6214e7792466b8 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Fri, 14 Jun 2019 22:06:13 -0400 Subject: [PATCH 04/14] add more explicitness --- text/0000-explicit-dependency-injection.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 64e673e311..485b49b646 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -53,7 +53,15 @@ function service(nameOrClass) { In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. -A new map can exist on the registry that can appropriately registers, unregisters, etc by class definition at the same time as the current registry behaves, allowing for backward compatibility with todays injection usage. +On the `Registry`, there already exists a reference to the class definition when registering an entry to the container. + +```ts +registry.register('model:user', Person, {singleton: false }); +registry.register('fruit:favorite', Orange); +registry.register('communication:main', Email, {singleton: false}); +``` + +A new map can exist on the registry that can appropriately be wired up to register, unregister, etc to handle the lookup-by-class-definition at the same time as the current registry behaves, allowing for backward compatibility with todays injection usage. ## How we teach this From 473a4f6f1d5f42a781543a41421404565fca0225 Mon Sep 17 00:00:00 2001 From: "L. Preston Sego III" Date: Sat, 15 Jun 2019 06:08:43 -0400 Subject: [PATCH 05/14] Update 0000-explicit-dependency-injection.md --- text/0000-explicit-dependency-injection.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 485b49b646..45f6c2c3a4 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -3,7 +3,7 @@ - RFC PR: https://github.com/emberjs/rfcs/pull/502 - Tracking: (leave this empty) -# Explicit Dependency Injection +# Explicit Service Injection ## Summary @@ -93,4 +93,4 @@ Eventually, we'll want to introduce a deprecation for string injections to minim - instantiated via `this.buildRegistry();` - registry is an instance of `Registry` - defined at `ember.js/packages/@ember/-internals/container/lib/registry.ts` - - in here is where the bulk of the implementation of this RFC would live? (along with updating the type signatures of the methods all the way up the call tree (where not inferred)) \ No newline at end of file + - in here is where the bulk of the implementation of this RFC would live? (along with updating the type signatures of the methods all the way up the call tree (where not inferred)) From 2e1d69971f76793b063b1cb82a930f97a109c63c Mon Sep 17 00:00:00 2001 From: "L. Preston Sego III" Date: Sat, 15 Jun 2019 07:04:12 -0400 Subject: [PATCH 06/14] Update 0000-explicit-dependency-injection.md --- text/0000-explicit-dependency-injection.md | 1 - 1 file changed, 1 deletion(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 45f6c2c3a4..3333485b5f 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -68,7 +68,6 @@ A new map can exist on the registry that can appropriately be wired up to regist Instead of using strings or inferred injections, the guides should be updated to use the Class definition of a service that it intends to inject. -Eventually, we'll want to introduce a deprecation for string injections to minimize different injection techniques. ## Drawbacks From 8052839beffbc8714efe06628c0e06dbc900417f Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Mon, 17 Jun 2019 17:43:16 -0400 Subject: [PATCH 07/14] Update 0000-explicit-dependency-injection.md --- text/0000-explicit-dependency-injection.md | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 3333485b5f..7df6b9ee7f 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -63,6 +63,44 @@ registry.register('communication:main', Email, {singleton: false}); A new map can exist on the registry that can appropriately be wired up to register, unregister, etc to handle the lookup-by-class-definition at the same time as the current registry behaves, allowing for backward compatibility with todays injection usage. +Examples: + +(also pardon the naming, as it's for demonstration of the _roles_ that each class definition can have) +```ts +// This behaves as the base implementation +class MyInterface extends Service { + @tracked foo = 0; + + add() { + this.foo++; + } +} + +// registration would look like it is today +appInstance.register(MyInterface, MyInterface); +// where we register MyInterface on the *key* MyInterface, +// just as today, it would look like +appInstance.register('service:my-interface', MyInterface); +``` +Now, where this _IS_ Dependency Injection, and how we aren't just using the concrete class all the time is where you can do things like this + +```ts +class MyImplementation extends MyInterface { + add() { + this.foo += 2; + } +} + +// both stubbing (in a test), or clobbering, would look the same +appInstance.register(MyInterface, MyImplementation); + +const service = appInstance.lookup(MyInstance); + +instanceof service === MyImplementation // true +instanceof service === MyInterface // true +``` + + ## How we teach this From fcffa48d27ef8d7ed64f4a6623d5a1031d2f8b10 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Mon, 17 Jun 2019 18:09:59 -0400 Subject: [PATCH 08/14] Syntax fixes --- text/0000-explicit-dependency-injection.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 7df6b9ee7f..54fa932b97 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -96,8 +96,8 @@ appInstance.register(MyInterface, MyImplementation); const service = appInstance.lookup(MyInstance); -instanceof service === MyImplementation // true -instanceof service === MyInterface // true +service instanceof MyImplementation // true +service instanceof MyInterface // true ``` From df410f5354c38318cd051b84d5b8ad35c0f5dbc3 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Wed, 19 Jun 2019 22:44:10 -0400 Subject: [PATCH 09/14] add a couple examples -- still needs some fleshing out --- text/0000-explicit-dependency-injection.md | 105 ++++++++++++++++++++- 1 file changed, 103 insertions(+), 2 deletions(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 54fa932b97..71d226cabc 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -51,7 +51,22 @@ function service(nameOrClass) { } ``` -In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. +In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. Though, when I classes is used for lookup, if there is no existing registration found, lookup _will register the class and instantiate it for you_. + +Consider: + +```ts +appInstance.register(MyClass, MyClass); +appInstance.lookup(MyClass); +``` + +is the same as +```ts +// no registration +appInstance.lookup(MyClass); +``` + +This will be most handy for decorators or other common abstractions that desire to interact with services (such as the router or store), but have direct access to them from the component or route context. On the `Registry`, there already exists a reference to the class definition when registering an entry to the container. @@ -67,7 +82,7 @@ Examples: (also pardon the naming, as it's for demonstration of the _roles_ that each class definition can have) ```ts -// This behaves as the base implementation +// This behaves as the base implementation / abstract class class MyInterface extends Service { @tracked foo = 0; @@ -100,12 +115,98 @@ service instanceof MyImplementation // true service instanceof MyInterface // true ``` +Logic will be added to the register method to ensure that the lookup type either is the same as the service instance's type or is an ancestor type. This will prevent the ability to register unrelated classes that would break the implied class hierarchy that is assumed with dependency injection. + +### Usage in testing + +#### Acceptance Tests + +Given that we have a service: +```ts +// app-name/services/my-service +export default class MyService extends Service { + someMethodThatIsInTheSuperClass() { + return 'not stubbed'; + } +} +``` + +And assuming that in a route there is a service injection like the following: +```ts +import Route from '@ember/routing/route'; +import ServiceToOverride from 'app-name/services/my-service'; + +export default class TheRoute extends Route { + @service(ServiceToOverride) myService; + // ... +} +``` + +During an acceptance test, it can be overwritten by doing the following: +```ts +import { module, test } from 'qunit'; +import { setupApplicationTest } from 'ember-qunit'; +import { getContext } from '@ember/test-helpers'; + +import ServiceToOverride from 'app-name/services/my-service'; + +module('Acceptance | test a thing', function(hooks) { + setupApplicationTest(hooks); + + hooks.beforeEach(function() { + let { owner } = getContext(); + + class MockService extends ServiceToOverride { + someMethodThatIsInTheSuperClass() { + return 'stubbed'; + } + } + + // register under the same 'key', ServiceToOverride + owner.register(ServiceToOverride, MockService); + + // re-inject on the-route + owner.application.inject('route:the-route', 'myService', ServiceToOverride); + }); +}); +``` + +### Integration Tests + +```ts +import { module, test } from 'qunit'; +import { setupRenderingTest } from 'ember-qunit'; +import { render } from '@ember/test-helpers'; +import hbs from 'htmlbars-inline-precompile'; +import Service from '@ember/service'; + +import LocationService from 'app-name/services/location'; + +class LocationStub extends LocationService { + getCurrentCity() { + return 'Indianapolis'; + } + + getCurrentCountry() { + return '???'; + } +}); + +module('Integration | Component | location-indicator', function(hooks) { + setupRenderingTest(hooks); + + hooks.beforeEach(function(assert) { + this.owner.register(LocationService, LocationStub); + }); +}); +``` ## How we teach this Instead of using strings or inferred injections, the guides should be updated to use the Class definition of a service that it intends to inject. +Ember Inspector and the unstable-ember-language-server will likely need to be updated to support this kind of lookup. ## Drawbacks From e653516f2afc5f9256e5805c0e2dacfc18db8fad Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Thu, 20 Jun 2019 09:16:31 -0400 Subject: [PATCH 10/14] Add @pzuraq's example --- text/0000-explicit-dependency-injection.md | 130 ++++++++++++++++----- 1 file changed, 101 insertions(+), 29 deletions(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 71d226cabc..0f2eab23b5 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -51,7 +51,7 @@ function service(nameOrClass) { } ``` -In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. Though, when I classes is used for lookup, if there is no existing registration found, lookup _will register the class and instantiate it for you_. +In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. Though, when a class is used for lookup, if there is no existing registration found, lookup _will register the class and instantiate it for you_. Consider: @@ -80,40 +80,112 @@ A new map can exist on the registry that can appropriately be wired up to regist Examples: -(also pardon the naming, as it's for demonstration of the _roles_ that each class definition can have) -```ts -// This behaves as the base implementation / abstract class -class MyInterface extends Service { - @tracked foo = 0; + - **service registration and override** + ```ts + class MyFooService extends Service { + @tracked foo = 0; - add() { - this.foo++; - } -} + add() { + this.foo++; + } + } -// registration would look like it is today -appInstance.register(MyInterface, MyInterface); -// where we register MyInterface on the *key* MyInterface, -// just as today, it would look like -appInstance.register('service:my-interface', MyInterface); -``` -Now, where this _IS_ Dependency Injection, and how we aren't just using the concrete class all the time is where you can do things like this + appInstance.register(MyFooService, MyFooService); + // this would register MyFooService on the *key* MyFooService, + // just as today, it would look like + appInstance.register('service:my-foo-service', MyFooService); + ``` + Now, where this _IS_ Dependency Injection, and how we aren't just using the concrete class all the time is where you can do things like this + + ```ts + // note that this must share ancestry with the registered service. + class MyFooOverrideService extends MyFooService { + add() { + this.foo += 2; + } + } -```ts -class MyImplementation extends MyInterface { - add() { - this.foo += 2; - } -} + // both stubbing (in a test), or clobbering, would look the same + appInstance.register(MyFooService, MyFooOverrideService); -// both stubbing (in a test), or clobbering, would look the same -appInstance.register(MyInterface, MyImplementation); + const service = appInstance.lookup(MyFooService); -const service = appInstance.lookup(MyInstance); + service instanceof MyFooOverrideService // true + service instanceof MyFooService // true + ``` -service instanceof MyImplementation // true -service instanceof MyInterface // true -``` + - **less typing for lazy registration** + ```ts + class MyFooService extends Service { + @tracked foo = 0; + + add() { + this.foo++; + } + } + + // the above service has not been registered yet + const myFooService = appInstance.lookup(MyFooService); + // first time lookup without registration will register for you. + + myFooService instanceof MyFooService // true + ``` + + - **typescript fastboot example with instance initializers** + + ```ts + // app/services/cookie/cookie-service.ts + abstract class CookieService { + abstract getValue(key: string): string {} + + abstract setValue(key: string, value: string): void {} + } + + // app/components/my-component.js + class MyComponent extends Component { + @service(CookieService) cookie; + } + + // app/services/cookie/fastboot-service.js + import CookieService from 'app-name/services/cookies/cookie-service'; + + class FastbootCookieService extends CookieService { + // ... + getValue(key: string) { + return ''; + } + } + + // app/services/cookie/browser-service.js + import CookieService from 'app-name/services/cookies/cookie-service'; + + class BrowserCookieService extends CookieService { + getValue(key: string) { + return browser.cookies.get({ + name: key, + url: window.location.href + }) + } + /// ... + } + + // app/instance-initializers/register-cookie-service; + import CookieService from 'app-name/services/cookies/cookie-service'; + import FastbootCookieService from 'app-name/services/cookies/fastboot-service'; + import BrowserCookieService from 'app-name/services/cookies/browser-service'; + + export function initialize(appInstance) { + cost fastboot = appInstance.lookup('service:fastboot'); + + if (fastboot.isFastBoot) { + appInstance.register(CookieService, FastbootCookieService); + } else { + appInstance.register(CookieService, BrowserCookieService); + } + } + + export default { initialize }; + ``` Logic will be added to the register method to ensure that the lookup type either is the same as the service instance's type or is an ancestor type. This will prevent the ability to register unrelated classes that would break the implied class hierarchy that is assumed with dependency injection. From 5c102b561945298d1b3a87c61223c5a5c0cbce1b Mon Sep 17 00:00:00 2001 From: NullVoxPopuli Date: Thu, 20 Jun 2019 09:17:46 -0400 Subject: [PATCH 11/14] Formatting --- text/0000-explicit-dependency-injection.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/text/0000-explicit-dependency-injection.md b/text/0000-explicit-dependency-injection.md index 0f2eab23b5..86aafd7d0a 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0000-explicit-dependency-injection.md @@ -142,6 +142,9 @@ Examples: } // app/components/my-component.js + import Component from '@glimmer/component'; + import { inject as service } from '@ember/service'; + class MyComponent extends Component { @service(CookieService) cookie; } @@ -150,10 +153,10 @@ Examples: import CookieService from 'app-name/services/cookies/cookie-service'; class FastbootCookieService extends CookieService { - // ... getValue(key: string) { return ''; } + // ... } // app/services/cookie/browser-service.js @@ -166,7 +169,7 @@ Examples: url: window.location.href }) } - /// ... + // ... } // app/instance-initializers/register-cookie-service; From 7f1505587bcfd6907bcd9edae2bc78a4e999801a Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:36:29 -0400 Subject: [PATCH 12/14] Update meta --- ... => 0502-explicit-dependency-injection.md} | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) rename text/{0000-explicit-dependency-injection.md => 0502-explicit-dependency-injection.md} (96%) diff --git a/text/0000-explicit-dependency-injection.md b/text/0502-explicit-dependency-injection.md similarity index 96% rename from text/0000-explicit-dependency-injection.md rename to text/0502-explicit-dependency-injection.md index 86aafd7d0a..f472aa2aa5 100644 --- a/text/0000-explicit-dependency-injection.md +++ b/text/0502-explicit-dependency-injection.md @@ -1,7 +1,18 @@ -- Start Date: 2019-06-14 -- Relevant Team(s): (fill this in with the [team(s)](README.md#relevant-teams) to which this RFC applies) -- RFC PR: https://github.com/emberjs/rfcs/pull/502 -- Tracking: (leave this empty) +--- +stage: accepted +start-date: 2019-06-14T00:00:00.000Z # In format YYYY-MM-DDT00:00:00.000Z +release-date: # In format YYYY-MM-DDT00:00:00.000Z +release-versions: +teams: # delete teams that aren't relevant + - data + - framework + - learning + - typescript +prs: + accepted: https://github.com/emberjs/rfcs/pull/502 +project-link: +suite: +--- # Explicit Service Injection From 8141315b17ba2e3ae52f5274e61f01fd61feac01 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Thu, 6 Aug 2026 17:15:31 -0400 Subject: [PATCH 13/14] More details from open issues --- text/0502-explicit-dependency-injection.md | 136 +++++++++++++++++++-- 1 file changed, 123 insertions(+), 13 deletions(-) diff --git a/text/0502-explicit-dependency-injection.md b/text/0502-explicit-dependency-injection.md index f472aa2aa5..388bd02908 100644 --- a/text/0502-explicit-dependency-injection.md +++ b/text/0502-explicit-dependency-injection.md @@ -24,20 +24,34 @@ Note: while this RFC mainly talks in terms in services, this applies to all inje ## Motivation -The main goal is to _use the platform_ and enable "go to definition" support from service definitions so developers can more easily discover the where and how their service is defined. +The main goal is to _use the platform_ and enable "go to definition" support from service definitions so developers can more easily discover the where and how their service is defined. Additionally, addons need to be able to create services that are not forced to be inluded in the initial JS bundle of applications -- if an addon is used in a split route, so, too, should their services. ## Detailed design -instead of: +The existing, not deprecated as a part of this RFC, ```ts @service notifications; @service('notifications') notifications; @service('messages/dispatcher') dispatcher; ``` -This will be the new default: + +has this behavior: +- service is not instantiated until accessed (even though the code for it is loaded) +- service is matched to a _file path_ within the `app/services` directory via custom build tooling +- usage with typescript requires that the developer import from the service file to `@service declare notifications: Notifications` - which is very repetitive +- service can be overridden later (if they have not previously been accessed) via `owner.register(key)` +- service can be resolved later without a decorator via `owner.lookup(key)` + + +This RFC proposes a new default, retaining the benefits of the above described features without the downsides: + ```ts -@service(NotificationsService) notifications; -@service(MessageDispatcherService) dispatcher; +@service(Notifications) notifications; +@service(MessageDispatcher) dispatcher; + +// or +notifications = service(this, Notifications); +dispatcher = service(this, MessageDispatcher); ``` This means that the shorthand syntax of using `@service` without a parameter should be discouraged, and the use of the service class should be used in its place. @@ -58,12 +72,82 @@ function service(nameOrClass) { return appInstance.lookup(`service:${name}`); } - return appInstance.lookup(nameOrClass); + + if (hasRegistration(appInstance, nameOrClass)) { + return appInstance.lookup(nameOrClass); + } + + // emulate maximal interface matching with the presently available services + let shape = shapeOf(nameOrClass); + + return lookupServiceByShape(appInstance, shape); } ``` In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. Though, when a class is used for lookup, if there is no existing registration found, lookup _will register the class and instantiate it for you_. + +Additionally we need to solve for libraries using services they are not the owners of is the "by-shape" lookup. Without doing so, there is a risk of "duplicate but same" services being included in an app's built output. + +### lookup forms + +#### decorators + +##### by string + +the existing behavior + +```js +class Demo { + @service('service') myService!: Service; + @service() service!: Service; + @service service!: Service; +} +``` + +##### by class / shape + +proposed by this RFC + +- retains lazy instantiation + +```ts +class Demo { + @service(Service) myService!: Service; +} +``` + +##### via function returning class / shape + +proposed by this RFC + +- retains lazy instantiation +- retains lazy definition / cycle-solving benefits + +```ts +class Demo { + @service(() => Service) myService!: Service; +} +``` + +#### without decorators + +proposed by this RFC + +- retains TypeScript types ahead of Decorators being shipped +- retains lazy instantiation +- retains lazy definition / cycle-solving benefits +- can be '#privateField' +- can be used in getter, function, etc + +```ts +class Demo { + myService = service(this, Service); +} +``` + +### lookup by exact class + Consider: ```ts @@ -73,7 +157,7 @@ appInstance.lookup(MyClass); is the same as ```ts -// no registration +// no registration needed, because MyClass could be in a dynamic bundle appInstance.lookup(MyClass); ``` @@ -109,7 +193,6 @@ Examples: Now, where this _IS_ Dependency Injection, and how we aren't just using the concrete class all the time is where you can do things like this ```ts - // note that this must share ancestry with the registered service. class MyFooOverrideService extends MyFooService { add() { this.foo += 2; @@ -154,7 +237,7 @@ Examples: // app/components/my-component.js import Component from '@glimmer/component'; - import { inject as service } from '@ember/service'; + import { service } from '@ember/service'; class MyComponent extends Component { @service(CookieService) cookie; @@ -203,6 +286,12 @@ Examples: Logic will be added to the register method to ensure that the lookup type either is the same as the service instance's type or is an ancestor type. This will prevent the ability to register unrelated classes that would break the implied class hierarchy that is assumed with dependency injection. +### lookup by shape + +- javascript does not have interfaces +- explicit services _can_ result in [module cycles](https://github.com/chancancode/ember-polaris-service/issues/18), due to how decorators work + because instantiation is lazy, however, we can devise a safe way to declare the service that is cycle-safe + ### Usage in testing #### Acceptance Tests @@ -292,19 +381,30 @@ module('Integration | Component | location-indicator', function(hooks) { Instead of using strings or inferred injections, the guides should be updated to use the Class definition of a service that it intends to inject. -Ember Inspector and the unstable-ember-language-server will likely need to be updated to support this kind of lookup. +The guides already cover usage of services, as well as techniques for setting up [any class for injection](https://guides.emberjs.com/release/in-depth-topics/native-classes-in-depth/#toc_using-injection) (albeit under "in depth topics") -- some of this probably could be expanded on but would be out of scope for this RFC. + +For libraries and apps wanting to incrementally migrate from string to explicit, we can provide instructions during a future depreaction-of-string-based-services RFC after a compat library exists, (proxying the string-instantiated service to the real one). + + ## Drawbacks - more verbose +- disruptive for libraries providing services ## Alternatives - - Convert the dependency injection lookup to a `WeakMap` + - in C# + asp.net core, dependency injections are resolved in the constructor of a class. This would be _even more verbose_ than what is being proposed in this RFC, as for many situations in Ember, the constructor can be omitted. - This would likely result in faster lookup, but would require more upfront work to change how the lookup method works on the `ApplicationInstance` / "owner". This _could_ be a separate RFC, but without benchmarking it's hard to say if this massive of a change would be worth it. + - just use `` + - many existing libraries for other ecosystems: very verbose, and use class-decorators, and/or constructor decorators -- which is more like C# + asp.net core. + + - some existing explorations + - https://github.com/chancancode/ember-polaris-service + - https://ember-primitives.pages.dev/6-utils/createService + a goal of the RFC is to signal to the community which approach will be supported and is safest. + ember-polaris-service did a lot of the exploration early, and ember-primitives' createService is a demonstration of how, if you really want, you don't need a service abstraction at all (leaning in to "don't mock", and "WeakMap on the owner is good") - - in C# + asp.net core, dependency injections are resolved in the constructor of a class. This would be _even more verbose_ than what is being proposed in this RFC, as for many situations in Ember, the constructor can be omitted. ## Unresolved Questions @@ -318,3 +418,13 @@ Ember Inspector and the unstable-ember-language-server will likely need to be up - registry is an instance of `Registry` - defined at `ember.js/packages/@ember/-internals/container/lib/registry.ts` - in here is where the bulk of the implementation of this RFC would live? (along with updating the type signatures of the methods all the way up the call tree (where not inferred)) + + +## Appendix + +- Previous Alternative: Convert the dependency injection lookup to a `WeakMap` + + This would likely result in faster lookup, but would require more upfront work to change how the lookup method works on the `ApplicationInstance` / "owner". This _could_ be a separate RFC, but without benchmarking it's hard to say if this massive of a change would be worth it. + + + Moved out of `Alternatives`, because this RFC should not explicitly recommend or unrecommend specific implementation styles. A `WeakMap` style of managing singletons tied to an owner is exactly the strategy that [`createService`](https://ember-primitives.pages.dev/6-utils/createService) from ember-primitives uses. From 4a633be68c7ace8d24056979df1e00bb92c65565 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Thu, 6 Aug 2026 19:03:04 -0400 Subject: [PATCH 14/14] Might need help with wordsmithing the rest of this --- text/0502-explicit-dependency-injection.md | 402 +++++++++++++++++---- 1 file changed, 330 insertions(+), 72 deletions(-) diff --git a/text/0502-explicit-dependency-injection.md b/text/0502-explicit-dependency-injection.md index 388bd02908..7f81cf803f 100644 --- a/text/0502-explicit-dependency-injection.md +++ b/text/0502-explicit-dependency-injection.md @@ -72,22 +72,16 @@ function service(nameOrClass) { return appInstance.lookup(`service:${name}`); } - - if (hasRegistration(appInstance, nameOrClass)) { - return appInstance.lookup(nameOrClass); - } - - // emulate maximal interface matching with the presently available services - let shape = shapeOf(nameOrClass); - - return lookupServiceByShape(appInstance, shape); + return appInstance.lookup(nameOrClass); } ``` In order for `lookup` to be able to take a class definition as an argument, there will need to be an alternative way to _lookup_ instances of services by the class. Though, when a class is used for lookup, if there is no existing registration found, lookup _will register the class and instantiate it for you_. -Additionally we need to solve for libraries using services they are not the owners of is the "by-shape" lookup. Without doing so, there is a risk of "duplicate but same" services being included in an app's built output. +Libraries using services they are not the owners of are not solved by this RFC; see +"out of scope: interface / shape matching". Such a library keeps using a string key, +which is what it does today. ### lookup forms @@ -105,7 +99,7 @@ class Demo { } ``` -##### by class / shape +##### by class proposed by this RFC @@ -117,7 +111,7 @@ class Demo { } ``` -##### via function returning class / shape +##### via function returning class proposed by this RFC @@ -135,7 +129,6 @@ class Demo { proposed by this RFC - retains TypeScript types ahead of Decorators being shipped -- retains lazy instantiation - retains lazy definition / cycle-solving benefits - can be '#privateField' - can be used in getter, function, etc @@ -146,6 +139,40 @@ class Demo { } ``` +**This form does not retain lazy instantiation.** A field initializer must evaluate +to something, so deferring means returning a stand-in. The only candidate (which is not being proposed) is a +`Proxy`, which: + +- throws on private fields: `this.#state` is a `TypeError` when the receiver is a + Proxy, and `#privateField` is listed above as a benefit of this form +- breaks identity: `this.myService === owner.lookup(Service)` is `false` +- shows as a Proxy in the debugger + +A `tracked()` read as `this.myService.value` is lazy and correct, but makes the return value more verbose to use, which would make the following not work + +site. The lazy non-decorator form is a getter: + +```ts +class Demo { + // resolves during construction + eager = service(this, Service); + // resolves on first access. + // repeat service() calls are idempontent. + get lazy() { return service(this, Service); } +} +``` + +#### overloading service in js/ts + +One export serves every form: + +| call | meaning | +| --- | --- | +| `service('name')` | today's behavior, unchanged | +| `service(Key)` | lazy decorator | +| `service(() => Key)` | lazy decorator, cycle-tolerant | +| `service(this, Key)` | resolve now | + ### lookup by exact class Consider: @@ -163,21 +190,11 @@ appInstance.lookup(MyClass); This will be most handy for decorators or other common abstractions that desire to interact with services (such as the router or store), but have direct access to them from the component or route context. -On the `Registry`, there already exists a reference to the class definition when registering an entry to the container. - -```ts -registry.register('model:user', Person, {singleton: false }); -registry.register('fruit:favorite', Orange); -registry.register('communication:main', Email, {singleton: false}); -``` - -A new map can exist on the registry that can appropriately be wired up to register, unregister, etc to handle the lookup-by-class-definition at the same time as the current registry behaves, allowing for backward compatibility with todays injection usage. - Examples: - **service registration and override** ```ts - class MyFooService extends Service { + class MyFooService { @tracked foo = 0; add() { @@ -186,9 +203,6 @@ Examples: } appInstance.register(MyFooService, MyFooService); - // this would register MyFooService on the *key* MyFooService, - // just as today, it would look like - appInstance.register('service:my-foo-service', MyFooService); ``` Now, where this _IS_ Dependency Injection, and how we aren't just using the concrete class all the time is where you can do things like this @@ -210,7 +224,7 @@ Examples: - **less typing for lazy registration** ```ts - class MyFooService extends Service { + class MyFooService { @tracked foo = 0; add() { @@ -228,12 +242,12 @@ Examples: - **typescript fastboot example with instance initializers** ```ts - // app/services/cookie/cookie-service.ts - abstract class CookieService { - abstract getValue(key: string): string {} + // app/services/cookies/cookie-service.ts + export abstract class CookieService { + abstract getValue(key: string): string | undefined; - abstract setValue(key: string, value: string): void {} - } + abstract setValue(key: string, value: string): void; + } // app/components/my-component.js import Component from '@glimmer/component'; @@ -243,20 +257,20 @@ Examples: @service(CookieService) cookie; } - // app/services/cookie/fastboot-service.js - import CookieService from 'app-name/services/cookies/cookie-service'; + // app/services/cookies/fastboot-service.ts + import { CookieService } from 'app-name/services/cookies/cookie-service'; - class FastbootCookieService extends CookieService { + export class FastbootCookieService extends CookieService { getValue(key: string) { - return ''; + return undefined; } // ... } - // app/services/cookie/browser-service.js - import CookieService from 'app-name/services/cookies/cookie-service'; + // app/services/cookies/browser-service.ts + import { CookieService } from 'app-name/services/cookies/cookie-service'; - class BrowserCookieService extends CookieService { + export class BrowserCookieService extends CookieService { getValue(key: string) { return browser.cookies.get({ name: key, @@ -266,15 +280,15 @@ Examples: // ... } - // app/instance-initializers/register-cookie-service; - import CookieService from 'app-name/services/cookies/cookie-service'; - import FastbootCookieService from 'app-name/services/cookies/fastboot-service'; - import BrowserCookieService from 'app-name/services/cookies/browser-service'; + // app/instance-initializers/register-cookie-service.ts + import { CookieService } from 'app-name/services/cookies/cookie-service'; + import { FastbootCookieService } from 'app-name/services/cookies/fastboot-service'; + import { BrowserCookieService } from 'app-name/services/cookies/browser-service'; export function initialize(appInstance) { - cost fastboot = appInstance.lookup('service:fastboot'); - - if (fastboot.isFastBoot) { + const fastboot = appInstance.lookup('service:fastboot'); + + if (fastboot?.isFastBoot) { appInstance.register(CookieService, FastbootCookieService); } else { appInstance.register(CookieService, BrowserCookieService); @@ -284,13 +298,180 @@ Examples: export default { initialize }; ``` + A key can be a pure contract with no runtime footprint at all. + +#### the hierarchy check + Logic will be added to the register method to ensure that the lookup type either is the same as the service instance's type or is an ancestor type. This will prevent the ability to register unrelated classes that would break the implied class hierarchy that is assumed with dependency injection. -### lookup by shape +```ts +appInstance.register(CookieService, SomethingUnrelated); +// Error: SomethingUnrelated is neither CookieService nor a subclass of it. +``` + +Note that this makes subclassing the only way to substitute an implementation, which +is the constraint an interface-matching followup would relax. + +#### registering after a key has been resolved + +Today this silently does nothing: the container has cached the instance and never +consults the new registration. The usual way to hit it is stubbing after `render`, +and the result is a test that passes for the wrong reason. Class-keyed registration +should assert instead: + +```ts +lookup(owner, Counter); +register(owner, Counter, StubCounter); +// Error: Counter has already been resolved on this owner. Register before the +// first lookup -- in a test, before render or visit. +``` + +#### `hasRegistration` does not survive the move to class keys + +`hasRegistration('service:foo')` answers "can the resolver find this name", and the +answer can be no. For a class key it is trivially yes for every key -- worst case the +key instantiates itself -- so the method becomes useless rather than different. + +Two separate predicates replace it: + +- is an explicit override bound for this key on this owner? +- has this key been resolved on this owner yet? + +The second is what the assertion above needs. + +### services no longer need a base class + +There is no name to resolve and no `create` contract to satisfy, so a service can be +a plain class: + +```ts +export default class Counter { + @tracked count = 0; + + constructor(owner: Owner) { + registerDestructor(this, () => this.cleanup()); + } + + increment = () => this.count++; +} +``` + +`InternalFactoryManager#create` currently asserts +`typeof this.class.create === 'function'`, so only `EmberObject` descendants can be +registered (or those simulating the create method from `EmberObject`). Class-keyed lookup needs both paths: + +- static `create` (an `EmberObject` descendant): instantiate through it, with the + owner set on the props object, as today +- otherwise: `new Impl(owner)`, then `setOwner(instance, owner)` + +Extending `Service` stays supported; it stops being mandatory. + +#### the owner is not available in a plain class's field initializers + +The owner is attached after the constructor returns: + +```ts +class Broken { + log = service(this, Logger); // Error: `this` has no owner yet +} + +class Works { + #logger: Logger; + + constructor(owner: Owner) { + this.#logger = service(owner, Logger); // inject from the parameter + } + + get other() { return service(this, Other); } // or resolve after construction +} +``` + +`EmberObject`-based services do not hit this, because `create(props)` sets the owner +before `init` runs. + +### out of scope: interface / shape matching - javascript does not have interfaces -- explicit services _can_ result in [module cycles](https://github.com/chancancode/ember-polaris-service/issues/18), due to how decorators work - because instantiation is lazy, however, we can devise a safe way to declare the service that is cycle-safe + +A library cannot name a key the *app* owns, which is +[ember-polaris-service#19](https://github.com/chancancode/ember-polaris-service/issues/19): +the library declares a dependency, the app supplies the implementation, and there is +no class both sides can import. + +> [!NOTE] +> For the ember-intl case, where a library _does_ depend on ember-intl, and uses their class key, since the app version of the intl service would _extend_ (and be required to extend) the intl service, because we have the hierarchy lookup, this use case still works with explicit service references. An important caveat is that the first access registers the instance -- so upon app boot, it should interact with the intl service, such as setting the default locale or reading the locale from localStorage / the URL. + +Matching a key structurally -- "any registered class with these members" -- would +close that gap, but it is a large feature with its own hazards (renaming a method +silently unbinds a provider; two candidates matching one key has no defensible +answer; `abstract` members leave nothing at runtime to match against), and it is not +needed to ship class keys. + +**Use a string key for this case.** `@service('transport')` continues to work +exactly as it does today, so nothing is lost relative to the status quo -- this RFC +adds a second way to name a dependency rather than replacing the first. A library +whose dependency is app-supplied keeps the string, and converts if and when a +followup RFC gives it something better. + +One constraint that does belong here, because it constrains the implementation: +class lookup must not fall back to *name* resolution. `lookup(FeatureFlags)` must not +resolve `app/services/feature-flags.ts` because the dasherized name happens to match, +or "go to definition" stops being trustworthy -- which is the motivation for the RFC. +Class keys and string names are separate namespaces that coexist. + +### module cycles + +ESM permits the cycle; what fails is reading a binding before the owning module has +finished evaluating, and a decorator *argument* is evaluated at class-definition time. + +```ts +// app/domain/cycle/editor.ts +import History from './history.ts'; + +export default class Editor { + @service(() => History) declare history: History; +} + +// app/domain/cycle/history.ts +import Editor from './editor.ts'; + +export default class History { + @service(() => Editor) declare editor: Editor; +} +``` + +**Both** sides need the thunk. Whichever module is imported first works with the +direct form, but which one that is depends on the app's import order rather than on +either file. The thunk is the default for mutually-dependent services, not an +advanced escape hatch. + +### TypeScript + +`lookup`'s return type is derived from the `DIRegistry` declaration-merging registry, +keyed on the string name, which is why every service in a typed app carries this: + +```ts +declare module '@ember/service' { + interface Registry { + 'feature-flags': FeatureFlags; + } +} +``` + +With a class key the instance type comes from the class, so the declaration merging +disappears, as does the `@service declare foo: Foo` duplication from Motivation: + +```ts +@service(FeatureFlags) declare flags: FeatureFlags; // annotation now redundant +``` + +Two signature notes: + +- `Key` must allow abstract constructors (`abstract new (owner: Owner) => T`); a + token that is only a contract is the most useful kind of key +- `hasRegistration` and `unregister` are not on the public `Owner` type today, they + live on the private `RegistryProxy`. The testing examples use them, so this RFC + needs to say whether they become public. ### Usage in testing @@ -299,7 +480,7 @@ Logic will be added to the register method to ensure that the lookup type either Given that we have a service: ```ts // app-name/services/my-service -export default class MyService extends Service { +export default class MyService { someMethodThatIsInTheSuperClass() { return 'not stubbed'; } @@ -321,7 +502,6 @@ During an acceptance test, it can be overwritten by doing the following: ```ts import { module, test } from 'qunit'; import { setupApplicationTest } from 'ember-qunit'; -import { getContext } from '@ember/test-helpers'; import ServiceToOverride from 'app-name/services/my-service'; @@ -329,8 +509,6 @@ module('Acceptance | test a thing', function(hooks) { setupApplicationTest(hooks); hooks.beforeEach(function() { - let { owner } = getContext(); - class MockService extends ServiceToOverride { someMethodThatIsInTheSuperClass() { return 'stubbed'; @@ -338,22 +516,22 @@ module('Acceptance | test a thing', function(hooks) { } // register under the same 'key', ServiceToOverride - owner.register(ServiceToOverride, MockService); - - // re-inject on the-route - owner.application.inject('route:the-route', 'myService', ServiceToOverride); + this.owner.register(ServiceToOverride, MockService); }); }); ``` +Registering under the key is the whole of it. There is no second `inject` step, +because the injection resolves through the owner on access rather than being assigned +at instantiation. The registration does have to happen before `visit`; see +"registering after a key has been resolved". + ### Integration Tests ```ts import { module, test } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; import { render } from '@ember/test-helpers'; -import hbs from 'htmlbars-inline-precompile'; -import Service from '@ember/service'; import LocationService from 'app-name/services/location'; @@ -365,17 +543,36 @@ class LocationStub extends LocationService { getCurrentCountry() { return '???'; } -}); +} module('Integration | Component | location-indicator', function(hooks) { setupRenderingTest(hooks); - hooks.beforeEach(function(assert) { + hooks.beforeEach(function() { this.owner.register(LocationService, LocationStub); }); }); ``` +The stub does not have to live anywhere in particular or be reachable by the +resolver -- it is a class in the test file. There is no name to get right and no +`app/services/` file to shadow. + +#### Unit tests + +A service with no owner-dependent behavior needs no setup: + +```ts +test('it counts', function (assert) { + const counter = new Counter(); + + counter.increment(); + + assert.strictEqual(counter.count, 1); +}); +``` + +Not available today, since `Service` has to be created through the container. ## How we teach this @@ -385,12 +582,30 @@ The guides already cover usage of services, as well as techniques for setting up For libraries and apps wanting to incrementally migrate from string to explicit, we can provide instructions during a future depreaction-of-string-based-services RFC after a compat library exists, (proxying the string-instantiated service to the real one). +Things the guides must cover, because each is a place reasonable code fails: + +- We should default to lazily-initialized forms +- `@service(Key)` is the default form (shortest, nicest for javascript). +- plain-class services cannot inject in field initializers -- teach the getter +- register before first lookup, which in a test means before `render`/`visit` + This doesn't exist today in the guides, as our guides on testing could use some work +- a dependency the app supplies, rather than the library, still uses a string key +The service directory only matters for services still reached by a string key. A +class-keyed service can live next to its consumers, since the import is the +registration. ## Drawbacks - more verbose - disruptive for libraries providing services +- the non-decorator form loses lazy instantiation +- class keys create module edges string keys did not, so cycles become possible where + they could not exist before. The thunk is a workaround, easy to forget, and + forgetting it fails at boot. +- class names are minified in production, so the container cache and the Ember + Inspector lose the readable names string keys give them +- both styles coexist ## Alternatives @@ -405,20 +620,24 @@ For libraries and apps wanting to incrementally migrate from string to explicit, a goal of the RFC is to signal to the community which approach will be supported and is safest. ember-polaris-service did a lot of the exploration early, and ember-primitives' createService is a demonstration of how, if you really want, you don't need a service abstraction at all (leaning in to "don't mock", and "WeakMap on the owner is good") + - symbol tokens instead of classes: `owner.lookup(TRANSPORT)`. Solves the same + coupling problem and avoids module cycles entirely, since a symbol has no + dependencies. Loses the thing the RFC is named for -- a symbol is not + ctrl+clickable to an implementation, only to its own declaration. + - shape matching in this RFC rather than a followup. Solves polaris-service#19 now, + at the cost of a much larger surface and a fuzzier one; see "out of scope". ## Unresolved Questions - These may be more for implementation, but as I was tracing how injections work.. it looks like the control flow path is: - - `@service` - - `appInstance.lookup` - - `engineInstance.lookup` (Application inherits from Engine) - - registry defined on `RegistryProxyMixin` - - instantiated via `this.buildRegistry();` - - registry is an instance of `Registry` - - defined at `ember.js/packages/@ember/-internals/container/lib/registry.ts` - - in here is where the bulk of the implementation of this RFC would live? (along with updating the type signatures of the methods all the way up the call tree (where not inferred)) + - Should `hasRegistration` accept a class at all? It may be string-only forever, + with two new predicates added alongside. + - What does the Ember Inspector show for a class-keyed instance in a production + build, where `Klass.name` is minified? + + - Does the `@service` decorator need to keep supporting assignment + (`this.myService = stub`)? ## Appendix @@ -426,5 +645,44 @@ For libraries and apps wanting to incrementally migrate from string to explicit, This would likely result in faster lookup, but would require more upfront work to change how the lookup method works on the `ApplicationInstance` / "owner". This _could_ be a separate RFC, but without benchmarking it's hard to say if this massive of a change would be worth it. - Moved out of `Alternatives`, because this RFC should not explicitly recommend or unrecommend specific implementation styles. A `WeakMap` style of managing singletons tied to an owner is exactly the strategy that [`createService`](https://ember-primitives.pages.dev/6-utils/createService) from ember-primitives uses. + + **Update:** this turned out to be *less* upfront work rather than more, because + it leaves `Registry` and the string path untouched instead of extending them. It + is now the recommended direction rather than a rejected alternative; see + "Implementation notes". + +- Where this has been validated + + "Detailed design" has been implemented and tested against `ember-source` 7 in a + playground app. + + The owner already *is* the container, so no parallel container is needed. The + mechanism is a `WeakMap` keyed on the owner, using only `getOwner` and `setOwner` + from `@ember/owner`. Instances are per-owner singletons because the outer key is + the owner, and are destroyed with the owner via `associateDestroyableChild`. + About 140 lines, including the decorator. + + + +- A class key needs no name + + And also, no resolution, and no resolver, so everything class-keyed can live in a `WeakMap` keyed on the owner: + + ```ts + const INSTANCES = new WeakMap>(); + const BINDINGS = new WeakMap>(); + ``` + + `Registry` and the string path are then untouched, which is also what keeps the two + namespaces separate by construction rather than by convention. This is the same strategy + [`createService`](https://ember-primitives.pages.dev/6-utils/createService) uses. + + What does need to change inside `container`: + + - `InternalFactoryManager#create` must not require a static `create` + - destruction has to cover class-keyed instances. `destroyDestroyables` calls + `value.destroy()` if it happens to exist, which is not enough for plain classes; + `associateDestroyableChild(owner, instance)` at instantiation handles it, and makes + `registerDestructor` work with no base class. +