From b017734e1604c49a590788776de8c004fa66eb46 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Sun, 4 Oct 2020 16:12:13 -0700 Subject: [PATCH 01/14] Deprecation for implicit injections --- text/0670-implicit-injection-deprecation.md | 119 ++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 text/0670-implicit-injection-deprecation.md diff --git a/text/0670-implicit-injection-deprecation.md b/text/0670-implicit-injection-deprecation.md new file mode 100644 index 0000000000..f9e5f4b270 --- /dev/null +++ b/text/0670-implicit-injection-deprecation.md @@ -0,0 +1,119 @@ +- Start Date: 2020-10-04 +- Relevant Team(s): Ember.js +- RFC PR: (after opening the RFC PR, update this with a link to it and update the file name) +- Tracking: (leave this empty) + +# Implicit Injection Deprecation + +## Summary + +This RFC seeks to deprecate implicit injections on arbitrary Ember Framework objects. This is commonly done via `owner.inject` in an +intializer or instance-initializer. + +A prevalent example of implicit injection is found in `ember-data` [injecting](https://github.com/emberjs/data/blob/4bd2b327c4cbca831f9e9f8bc6b497200a212f9b/packages/-ember-data/addon/setup-container.js) their default `store` into all +Ember.Route and Ember.Controller factory objects. Here is a pruned example of how this looks. + +```js +// app/initializers/store-inject.js +export function initialize(application) { + application.inject('route', 'store', 'service:store'); + application.inject('controller', 'store', 'service:store'); +} + +export default { + name: 'store-inject', + initialize, +}; +``` + +```js +export default class PostRoute extends Route { + // This proposal seeks to make this service injection explicit + // Currently users do no need to specify this injection if ember-data is installed + // @service store; + + model() { + return this.store.findRecord('post', 1); + } +} +``` + +Ensuring a property is present on Ember's Framework objects should be explicitly defined. This will allow the Ember ecosystem to +further progress the framework while easing the learning curve for all developers. + +## Motivation + +Implicit injections have long confused developers onboarding Ember projects. Without an +explicit injection visible on the class body, developers start to wonder how certain properties got there. +We can infer from a common implicit injection in `ember-data` to reason about some of the downsides this presents. +If a project has `ember-data` installed, an initializer at runtime registers the `@ember-data/store` on both +Ember.Route and Ember.Controller factory objects. This is a nice convenience for a project with many routes +that are used for data fetching. However, it leads to many inconveniences that hinder learning and advancement +of the framework. This includes: + +1. By entangling specific objects with the whole dependency tree, we might be preventing incremental adoption. + For example, `ember-data` has laid out its plan for adopting only specific packages in [Project Trim](https://github.com/emberjs/data/issues/6166). + However, because `store` is injected by default on all Ember.Route and Ember.Controller factory objects, + a user cannot easily opt out of `@ember-data/store`. This is a common pattern found in other communities as well. + With previous JetBrains IDE installations, adding plugins required a reload. However, by removing a contructor function + that injected all the dependencies at once, they were able to avoid reload after installation of a plugin. + +2. Eager initialization of the dependency tree from which the property came from prevents tree shaking and incurs a + performance hit. If you app does not need `ember-data` to render the entry route, their use of `owner.inject` will + take up more CPU cycles than necessary, hurting common user facing metrics. + +3. Eager initialization makes it hard to users to write tests that need to take advantage of a stub. + This is often a silent and annoying hurdle that users have to get around when writing tests. + Moreover, users moving from Ember 2.x to 3.x may see new behaviour between each test run. + When initializers run has changed and previous attempts to stub out the injected property may not work. + +4. Overt injection on commonly used framework objects can lead to deviation of project specific styleguides. For instance, + injecting the `router` on the Ember.Component factory objects will certainly lead to instances where the `router` is explicitly + injected and other instances where is it implicitly injected. + +5. Users have come to associate `Route.model()` with a hook that returns `ember-data` models in the absense of explicit injection. + While this can be true, it is not wholly true. New patterns of data loading are becoming + accepted in the community including opting to fetch data in a Component. + + +## Detailed design + +Removing implicit injections is not possible until we first issue deprecations for users. +This helps us maintain our strict semver commitment to not completing a major version bump and +removing APIs without a deprecation path. + +### Deprecation + +The first step will be issuing the deprecation. This can be accomplished by installing a native getter +and setter on the target object that is receiving the property. Whenever the property is accessed, +we will check if the property is explicitly injected. If it isn't, we will issue a deprecation with an +until value, id and url to read more about this deprecation. + +### Removal + +Ember 4.0 will remove the ability to inject arbitrary properties on Ember's Framework objects. + +## How we teach this + +The API docs would need to be overhauld in a few spots. First, we need to remove the [docs](https://guides.emberjs.com/release/applications/dependency-injection/#toc_factory-injections) about Factory injections. +Second, we need to detail explicit injection where currently relying on implicit injection. Many examples show fetching data +via `this.store` but do not specifiy how `this.store` arrived as a property on the Route. Also apparent +disclaimers for people visiting the `ember-data` docs that explicit injection of `store` is necessary +and has deviated from past behaviour is likely prudent. + +The default blueprints when generating a new Ember application will not change. + +## Alternatives + +- Continue with default store injection +- Provide application level super class with a default store injection provided by `ember-data` and other common examples in the community. +- Allow injection on a specific factory with a single backing Class instead of a generic factory. + +## Open Questions + +- Should we simply replace the [docs](https://guides.emberjs.com/release/applications/dependency-injection/#toc_factory-injections) on Factory Injections without an alternative? + +## Related links and RFCs +- [Deprecate defaultStore located at `Route.store`](https://github.com/emberjs/rfcs/issues/377) +- [Pre-RFC: Deprecate implicit injections (owner.inject)](https://github.com/emberjs/rfcs/issues/508) +- [Deprecate implicit record loading in routes](https://github.com/emberjs/rfcs/issues/557) From 88b47abe5e510bff676e0acabc355bd945132ad7 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Mon, 5 Oct 2020 16:15:00 -0700 Subject: [PATCH 02/14] Add phases --- text/0670-implicit-injection-deprecation.md | 28 +++++++++++++-------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/text/0670-implicit-injection-deprecation.md b/text/0670-implicit-injection-deprecation.md index f9e5f4b270..73523366f6 100644 --- a/text/0670-implicit-injection-deprecation.md +++ b/text/0670-implicit-injection-deprecation.md @@ -80,18 +80,26 @@ of the framework. This includes: Removing implicit injections is not possible until we first issue deprecations for users. This helps us maintain our strict semver commitment to not completing a major version bump and -removing APIs without a deprecation path. +removing APIs without a deprecation path. As a result, it will take us 3 phases to remove `owner.inject`. -### Deprecation +### 1. Deprecate implicit injection on target object -The first step will be issuing the deprecation. This can be accomplished by installing a native getter -and setter on the target object that is receiving the property. Whenever the property is accessed, -we will check if the property is explicitly injected. If it isn't, we will issue a deprecation with an -until value, id and url to read more about this deprecation. +The first step will be issuing a deprecation on the target factory object that is receiving a property. This can be +accomplished by installing a native getter and setter on the target. Whenever the property is accessed, we will +check if the property is explicitly injected. If it isn't, we will issue a deprecation with an until value, id and +url to read more about this deprecation. -### Removal +### 2. Deprecate `owner.inject` -Ember 4.0 will remove the ability to inject arbitrary properties on Ember's Framework objects. +The first phase did not actually deprecate the "use" of `owner.inject`. However, we need to deprecate +it's use directly before removing. This will follow the standard deprecation path. + +### 3. Profit! + +Ember 4.0 will remove the ability to utilize `owner.inject` to inject arbitrary properties on Ember's Framework objects. + +It is important to consider the timeline of these three phases. Many addons and apps will need to make minor and major +changes to their codebase. We need to consider this as we move through each phase. ## How we teach this @@ -105,9 +113,9 @@ The default blueprints when generating a new Ember application will not change. ## Alternatives -- Continue with default store injection +- Continue with use of `owner.inject` but overhaul docs and recommend explicit injection. - Provide application level super class with a default store injection provided by `ember-data` and other common examples in the community. -- Allow injection on a specific factory with a single backing Class instead of a generic factory. +- Allow injection on a specific factory with a single backing Class instead of a generic factory object (e.g. `route` vs `route.index`). ## Open Questions From cf461b21c0c770c0d0fb642a5e4e203da00619cb Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Mon, 5 Oct 2020 20:21:34 -0700 Subject: [PATCH 03/14] Improve with comments about wny phase 1 --- ...=> 0680-implicit-injection-deprecation.md} | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) rename text/{0670-implicit-injection-deprecation.md => 0680-implicit-injection-deprecation.md} (82%) diff --git a/text/0670-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md similarity index 82% rename from text/0670-implicit-injection-deprecation.md rename to text/0680-implicit-injection-deprecation.md index 73523366f6..4ee4df9498 100644 --- a/text/0670-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -54,21 +54,21 @@ of the framework. This includes: 1. By entangling specific objects with the whole dependency tree, we might be preventing incremental adoption. For example, `ember-data` has laid out its plan for adopting only specific packages in [Project Trim](https://github.com/emberjs/data/issues/6166). However, because `store` is injected by default on all Ember.Route and Ember.Controller factory objects, - a user cannot easily opt out of `@ember-data/store`. This is a common pattern found in other communities as well. + a user cannot easily opt out of `@ember-data/store`. This is a common problem space in other communities as well. With previous JetBrains IDE installations, adding plugins required a reload. However, by removing a contructor function that injected all the dependencies at once, they were able to avoid reload after installation of a plugin. 2. Eager initialization of the dependency tree from which the property came from prevents tree shaking and incurs a - performance hit. If you app does not need `ember-data` to render the entry route, their use of `owner.inject` will + performance hit. For example, if you app does not need `ember-data` to render the entry route, the `store` injection will take up more CPU cycles than necessary, hurting common user facing metrics. 3. Eager initialization makes it hard to users to write tests that need to take advantage of a stub. This is often a silent and annoying hurdle that users have to get around when writing tests. - Moreover, users moving from Ember 2.x to 3.x may see new behaviour between each test run. - When initializers run has changed and previous attempts to stub out the injected property may not work. + Moreover, this is especially apparent for users moving to Ember's new testing APIs in [RFC 232](https://github.com/emberjs/rfcs/blob/master/text/0232-simplify-qunit-testing-api.md) and [RFC 268](https://github.com/emberjs/rfcs/blob/master/text/0268-acceptance-testing-refactor.md), + ensuring instance initializers run before each test. -4. Overt injection on commonly used framework objects can lead to deviation of project specific styleguides. For instance, - injecting the `router` on the Ember.Component factory objects will certainly lead to instances where the `router` is explicitly +4. Injection on commonly used framework objects can lead to deviation of project specific styleguides. For instance, + injecting the `router` on the Ember.Component factory object will certainly lead to instances where the `router` is explicitly injected and other instances where is it implicitly injected. 5. Users have come to associate `Route.model()` with a hook that returns `ember-data` models in the absense of explicit injection. @@ -78,21 +78,21 @@ of the framework. This includes: ## Detailed design -Removing implicit injections is not possible until we first issue deprecations for users. +Removing implicit injections is not possible until we first issue deprecations. This helps us maintain our strict semver commitment to not completing a major version bump and removing APIs without a deprecation path. As a result, it will take us 3 phases to remove `owner.inject`. ### 1. Deprecate implicit injection on target object -The first step will be issuing a deprecation on the target factory object that is receiving a property. This can be -accomplished by installing a native getter and setter on the target. Whenever the property is accessed, we will -check if the property is explicitly injected. If it isn't, we will issue a deprecation with an until value, id and -url to read more about this deprecation. +The first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation +on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter +and setter on the target. Whenever the property is accessed, we will check if the property is explicitly injected. If it isn't, +we will issue a deprecation with an until value, id and url to read more about this deprecation. ### 2. Deprecate `owner.inject` -The first phase did not actually deprecate the "use" of `owner.inject`. However, we need to deprecate -it's use directly before removing. This will follow the standard deprecation path. +The first phase did not actually deprecate the "use" of `owner.inject`. As a result, we need to deprecate +it's use directly before removing. ### 3. Profit! From 6b729f821483cb7d04bab53931031110a8b9fd25 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 6 Nov 2020 10:00:59 -0800 Subject: [PATCH 04/14] Update text/0680-implicit-injection-deprecation.md Co-authored-by: Robert Jackson --- text/0680-implicit-injection-deprecation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index 4ee4df9498..2f928887a0 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -43,7 +43,7 @@ further progress the framework while easing the learning curve for all developer ## Motivation -Implicit injections have long confused developers onboarding Ember projects. Without an +Implicit injections have long confused developers onboarding into Ember projects. Without an explicit injection visible on the class body, developers start to wonder how certain properties got there. We can infer from a common implicit injection in `ember-data` to reason about some of the downsides this presents. If a project has `ember-data` installed, an initializer at runtime registers the `@ember-data/store` on both From c0609c432f3afa543182ac9e40f818e2e601d0a0 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 6 Nov 2020 10:01:13 -0800 Subject: [PATCH 05/14] Update text/0680-implicit-injection-deprecation.md Co-authored-by: Robert Jackson --- text/0680-implicit-injection-deprecation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index 2f928887a0..f6d53b52e3 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -11,7 +11,7 @@ This RFC seeks to deprecate implicit injections on arbitrary Ember Framework obj intializer or instance-initializer. A prevalent example of implicit injection is found in `ember-data` [injecting](https://github.com/emberjs/data/blob/4bd2b327c4cbca831f9e9f8bc6b497200a212f9b/packages/-ember-data/addon/setup-container.js) their default `store` into all -Ember.Route and Ember.Controller factory objects. Here is a pruned example of how this looks. +`@ember/routing/route` and `@ember/controller` factory objects. Here is a pruned example of how this looks. ```js // app/initializers/store-inject.js From 5f13fe303289c4d0d72690183e9d6f41e232da71 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 6 Nov 2020 10:52:01 -0800 Subject: [PATCH 06/14] Some feedback updates --- text/0680-implicit-injection-deprecation.md | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index f6d53b52e3..6bd2309be1 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -10,7 +10,7 @@ This RFC seeks to deprecate implicit injections on arbitrary Ember Framework objects. This is commonly done via `owner.inject` in an intializer or instance-initializer. -A prevalent example of implicit injection is found in `ember-data` [injecting](https://github.com/emberjs/data/blob/4bd2b327c4cbca831f9e9f8bc6b497200a212f9b/packages/-ember-data/addon/setup-container.js) their default `store` into all +A prevalent example of implicit injection is found in `ember-data` [injecting](https://github.com/emberjs/data/blob/4bd2b327c4cbca831f9e9f8bc6b497200a212f9b/packages/-ember-data/addon/setup-container.js) their default `@ember-data/store` into all `@ember/routing/route` and `@ember/controller` factory objects. Here is a pruned example of how this looks. ```js @@ -47,19 +47,19 @@ Implicit injections have long confused developers onboarding into Ember projects explicit injection visible on the class body, developers start to wonder how certain properties got there. We can infer from a common implicit injection in `ember-data` to reason about some of the downsides this presents. If a project has `ember-data` installed, an initializer at runtime registers the `@ember-data/store` on both -Ember.Route and Ember.Controller factory objects. This is a nice convenience for a project with many routes +`@ember/routing/route` and `@ember/controller` factory objects. This is a nice convenience for a project with many routes that are used for data fetching. However, it leads to many inconveniences that hinder learning and advancement of the framework. This includes: 1. By entangling specific objects with the whole dependency tree, we might be preventing incremental adoption. For example, `ember-data` has laid out its plan for adopting only specific packages in [Project Trim](https://github.com/emberjs/data/issues/6166). - However, because `store` is injected by default on all Ember.Route and Ember.Controller factory objects, + However, because `@ember-data/store` is injected by default on all `@ember/routing/route` and `@ember/controller` factory objects, a user cannot easily opt out of `@ember-data/store`. This is a common problem space in other communities as well. With previous JetBrains IDE installations, adding plugins required a reload. However, by removing a contructor function that injected all the dependencies at once, they were able to avoid reload after installation of a plugin. 2. Eager initialization of the dependency tree from which the property came from prevents tree shaking and incurs a - performance hit. For example, if you app does not need `ember-data` to render the entry route, the `store` injection will + performance hit. For example, if you app does not need `ember-data` to render the entry route, the `@ember-data/store` injection will take up more CPU cycles than necessary, hurting common user facing metrics. 3. Eager initialization makes it hard to users to write tests that need to take advantage of a stub. @@ -68,7 +68,7 @@ of the framework. This includes: ensuring instance initializers run before each test. 4. Injection on commonly used framework objects can lead to deviation of project specific styleguides. For instance, - injecting the `router` on the Ember.Component factory object will certainly lead to instances where the `router` is explicitly + injecting the `router` on the `@ember/component` factory object will certainly lead to instances where the `router` is explicitly injected and other instances where is it implicitly injected. 5. Users have come to associate `Route.model()` with a hook that returns `ember-data` models in the absense of explicit injection. @@ -87,7 +87,9 @@ removing APIs without a deprecation path. As a result, it will take us 3 phases The first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter and setter on the target. Whenever the property is accessed, we will check if the property is explicitly injected. If it isn't, -we will issue a deprecation with an until value, id and url to read more about this deprecation. +we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will +not issue this deprecation and continue assigning the property. + ### 2. Deprecate `owner.inject` @@ -106,7 +108,7 @@ changes to their codebase. We need to consider this as we move through each phas The API docs would need to be overhauld in a few spots. First, we need to remove the [docs](https://guides.emberjs.com/release/applications/dependency-injection/#toc_factory-injections) about Factory injections. Second, we need to detail explicit injection where currently relying on implicit injection. Many examples show fetching data via `this.store` but do not specifiy how `this.store` arrived as a property on the Route. Also apparent -disclaimers for people visiting the `ember-data` docs that explicit injection of `store` is necessary +disclaimers for people visiting the `ember-data` docs that explicit injection of `@ember-data/store` is necessary and has deviated from past behaviour is likely prudent. The default blueprints when generating a new Ember application will not change. @@ -114,7 +116,7 @@ The default blueprints when generating a new Ember application will not change. ## Alternatives - Continue with use of `owner.inject` but overhaul docs and recommend explicit injection. -- Provide application level super class with a default store injection provided by `ember-data` and other common examples in the community. +- Provide application level super class with a default `@ember-data/store` injection provided by `ember-data` and other common examples in the community. - Allow injection on a specific factory with a single backing Class instead of a generic factory object (e.g. `route` vs `route.index`). ## Open Questions From a670a751bba028434490c1187fb4f14e8ead26eb Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 6 Nov 2020 10:52:21 -0800 Subject: [PATCH 07/14] Update text/0680-implicit-injection-deprecation.md Co-authored-by: Richard Viney --- text/0680-implicit-injection-deprecation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index 6bd2309be1..8da2ea663e 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -55,7 +55,7 @@ of the framework. This includes: For example, `ember-data` has laid out its plan for adopting only specific packages in [Project Trim](https://github.com/emberjs/data/issues/6166). However, because `@ember-data/store` is injected by default on all `@ember/routing/route` and `@ember/controller` factory objects, a user cannot easily opt out of `@ember-data/store`. This is a common problem space in other communities as well. - With previous JetBrains IDE installations, adding plugins required a reload. However, by removing a contructor function + With previous JetBrains IDE installations, adding plugins required a reload. However, by removing a constructor function that injected all the dependencies at once, they were able to avoid reload after installation of a plugin. 2. Eager initialization of the dependency tree from which the property came from prevents tree shaking and incurs a From 6cd9f375d6cc1cb8ede11a422b2c80ec8b10d26f Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 6 Nov 2020 10:52:30 -0800 Subject: [PATCH 08/14] Update text/0680-implicit-injection-deprecation.md Co-authored-by: Bert De Block --- text/0680-implicit-injection-deprecation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index 8da2ea663e..c6fb6f115d 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -29,7 +29,7 @@ export default { ```js export default class PostRoute extends Route { // This proposal seeks to make this service injection explicit - // Currently users do no need to specify this injection if ember-data is installed + // Currently users do not need to specify this injection if ember-data is installed // @service store; model() { From a340a658e24882a5f9f9c0230d3672b33f92f956 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 6 Nov 2020 13:02:08 -0800 Subject: [PATCH 09/14] clarifications on how to get around deprecations --- text/0680-implicit-injection-deprecation.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index c6fb6f115d..598ca092b9 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -90,12 +90,18 @@ and setter on the target. Whenever the property is accessed, we will check if th we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will not issue this deprecation and continue assigning the property. +To avoid this deprecation, you will need to explicitly add the injected property to the target object. As an example, you can add +`@service store` to your route or controller to avert this deprecation. + ### 2. Deprecate `owner.inject` The first phase did not actually deprecate the "use" of `owner.inject`. As a result, we need to deprecate it's use directly before removing. +For users and or addons, you may be able to remove `owner.inject` to avoid this deprecation. However, for libraries like `@ember/data`, we will not be able to +remove until `@ember/data` v4. + ### 3. Profit! Ember 4.0 will remove the ability to utilize `owner.inject` to inject arbitrary properties on Ember's Framework objects. From 9d65b2ede1fd8b11534e340d9aede0b561da6ac4 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Thu, 19 Nov 2020 20:56:11 -0800 Subject: [PATCH 10/14] Address moar comments --- text/0680-implicit-injection-deprecation.md | 72 ++++++--------------- 1 file changed, 18 insertions(+), 54 deletions(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index 598ca092b9..a4eff90289 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -7,11 +7,9 @@ ## Summary -This RFC seeks to deprecate implicit injections on arbitrary Ember Framework objects. This is commonly done via `owner.inject` in an -intializer or instance-initializer. +This RFC seeks to deprecate implicit injections on arbitrary Ember Framework objects. This is commonly done via `owner.inject` in an intializer or instance-initializer. -A prevalent example of implicit injection is found in `ember-data` [injecting](https://github.com/emberjs/data/blob/4bd2b327c4cbca831f9e9f8bc6b497200a212f9b/packages/-ember-data/addon/setup-container.js) their default `@ember-data/store` into all -`@ember/routing/route` and `@ember/controller` factory objects. Here is a pruned example of how this looks. +A prevalent example of implicit injection is found in `ember-data` [injecting](https://github.com/emberjs/data/blob/4bd2b327c4cbca831f9e9f8bc6b497200a212f9b/packages/-ember-data/addon/setup-container.js) their default `@ember-data/store` into all `@ember/routing/route` and `@ember/controller` factory objects. Here is a pruned example of how this looks. ```js // app/initializers/store-inject.js @@ -38,84 +36,50 @@ export default class PostRoute extends Route { } ``` -Ensuring a property is present on Ember's Framework objects should be explicitly defined. This will allow the Ember ecosystem to -further progress the framework while easing the learning curve for all developers. +Ensuring a property is present on Ember's Framework objects should be explicitly defined. This will allow the Ember ecosystem to further progress the framework while easing the learning curve for all developers. ## Motivation -Implicit injections have long confused developers onboarding into Ember projects. Without an -explicit injection visible on the class body, developers start to wonder how certain properties got there. -We can infer from a common implicit injection in `ember-data` to reason about some of the downsides this presents. -If a project has `ember-data` installed, an initializer at runtime registers the `@ember-data/store` on both -`@ember/routing/route` and `@ember/controller` factory objects. This is a nice convenience for a project with many routes -that are used for data fetching. However, it leads to many inconveniences that hinder learning and advancement -of the framework. This includes: +Implicit injections have long confused developers onboarding into Ember projects. Without an explicit injection visible on the class body, developers start to wonder how certain properties got there. We can infer from a common implicit injection in `ember-data` to reason about some of the downsides this presents. If a project has `ember-data` installed, an initializer at runtime registers the `@ember-data/store` on both +`@ember/routing/route` and `@ember/controller` factory objects. This is a nice convenience for a project with many routes that are used for data fetching. However, it leads to many inconveniences that hinder learning and advancement of the framework. This includes: -1. By entangling specific objects with the whole dependency tree, we might be preventing incremental adoption. - For example, `ember-data` has laid out its plan for adopting only specific packages in [Project Trim](https://github.com/emberjs/data/issues/6166). - However, because `@ember-data/store` is injected by default on all `@ember/routing/route` and `@ember/controller` factory objects, - a user cannot easily opt out of `@ember-data/store`. This is a common problem space in other communities as well. - With previous JetBrains IDE installations, adding plugins required a reload. However, by removing a constructor function - that injected all the dependencies at once, they were able to avoid reload after installation of a plugin. +1. By entangling specific objects with the whole dependency tree, we might be preventing incremental adoption. For example, `ember-data` has laid out its plan for adopting only specific packages in [Project Trim](https://github.com/emberjs/data/issues/6166). However, because `@ember-data/store` is injected by default on all `@ember/routing/route` and `@ember/controller` factory objects, a user cannot easily opt out of `@ember-data/store`. This is a common problem space in other communities as well. With previous JetBrains IDE installations, adding plugins required a reload. However, by removing a constructor function that injected all the dependencies at once, they were able to avoid reload after installation of a plugin. -2. Eager initialization of the dependency tree from which the property came from prevents tree shaking and incurs a - performance hit. For example, if you app does not need `ember-data` to render the entry route, the `@ember-data/store` injection will - take up more CPU cycles than necessary, hurting common user facing metrics. +2. Eager initialization of the dependency tree from which the property came from prevents tree shaking and incurs a performance hit. For example, if you app does not need `ember-data` to render the entry route, the `@ember-data/store` injection will take up more CPU cycles than necessary, hurting common user facing metrics. -3. Eager initialization makes it hard to users to write tests that need to take advantage of a stub. - This is often a silent and annoying hurdle that users have to get around when writing tests. - Moreover, this is especially apparent for users moving to Ember's new testing APIs in [RFC 232](https://github.com/emberjs/rfcs/blob/master/text/0232-simplify-qunit-testing-api.md) and [RFC 268](https://github.com/emberjs/rfcs/blob/master/text/0268-acceptance-testing-refactor.md), - ensuring instance initializers run before each test. +3. Eager initialization makes it hard to users to write tests that need to take advantage of a stub. This is often a silent and annoying hurdle that users have to get around when writing tests. Moreover, this is especially apparent for users moving to Ember's new testing APIs in [RFC 232](https://github.com/emberjs/rfcs/blob/master/text/0232-simplify-qunit-testing-api.md) and [RFC 268](https://github.com/emberjs/rfcs/blob/master/text/0268-acceptance-testing-refactor.md), ensuring instance initializers run before each test. -4. Injection on commonly used framework objects can lead to deviation of project specific styleguides. For instance, - injecting the `router` on the `@ember/component` factory object will certainly lead to instances where the `router` is explicitly - injected and other instances where is it implicitly injected. +4. Injection on commonly used framework objects can lead to deviation of project specific styleguides. For instance, injecting the `router` on the `@ember/component` factory object will certainly lead to instances where the `router` is explicitly injected and other instances where is it implicitly injected. -5. Users have come to associate `Route.model()` with a hook that returns `ember-data` models in the absense of explicit injection. - While this can be true, it is not wholly true. New patterns of data loading are becoming - accepted in the community including opting to fetch data in a Component. +5. Users have come to associate `Route.model()` with a hook that returns `ember-data` models in the absense of explicit injection. While this can be true, it is not wholly true. New patterns of data loading are becoming accepted in the community including opting to fetch data in a Component. ## Detailed design -Removing implicit injections is not possible until we first issue deprecations. -This helps us maintain our strict semver commitment to not completing a major version bump and -removing APIs without a deprecation path. As a result, it will take us 3 phases to remove `owner.inject`. +Removing implicit injections is not possible until we first issue deprecations. For example, `@ember/data` cannot remove its `owner.inject` without it being a breaking change. In doing so, deprecating first helps us maintain our strict semver commitment to not completing a major version bump and removing APIs without a deprecation path. As a result, it will take us 3 phases to remove `owner.inject`. ### 1. Deprecate implicit injection on target object -The first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation -on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter -and setter on the target. Whenever the property is accessed, we will check if the property is explicitly injected. If it isn't, -we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will -not issue this deprecation and continue assigning the property. - -To avoid this deprecation, you will need to explicitly add the injected property to the target object. As an example, you can add -`@service store` to your route or controller to avert this deprecation. +Simply deprecating `owner.inject` is not sufficient because it is very difficult to detect if you are relying on implicit injection. As a result, the first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter and setter on the target. Whenever the property is accessed, we will check if the property is explicitly injected. If it isn't, we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will not issue this deprecation and continue assigning the property. +To avoid this deprecation, you will need to explicitly add the injected property to the target object. As an example, you can add `@service store` to your route or controller to avert this deprecation resulting from `@ember/data`'s use of `owner.inject`. ### 2. Deprecate `owner.inject` -The first phase did not actually deprecate the "use" of `owner.inject`. As a result, we need to deprecate -it's use directly before removing. +The first phase did not actually deprecate the use of `owner.inject`. As a result, we need to deprecate it's use directly before removing. -For users and or addons, you may be able to remove `owner.inject` to avoid this deprecation. However, for libraries like `@ember/data`, we will not be able to -remove until `@ember/data` v4. +For users and or addons, you may be able to remove `owner.inject` to avoid this deprecation. However, for libraries like `@ember/data`, we will not be able to remove until `@ember/data` v4. ### 3. Profit! Ember 4.0 will remove the ability to utilize `owner.inject` to inject arbitrary properties on Ember's Framework objects. -It is important to consider the timeline of these three phases. Many addons and apps will need to make minor and major -changes to their codebase. We need to consider this as we move through each phase. + +It is important to consider the timeline of these three phases. Each step will consist of a minimum of one release cycle. Many addons and apps will need to make minor and major changes to their codebase. We need to consider this as we move through each phase. ## How we teach this -The API docs would need to be overhauld in a few spots. First, we need to remove the [docs](https://guides.emberjs.com/release/applications/dependency-injection/#toc_factory-injections) about Factory injections. -Second, we need to detail explicit injection where currently relying on implicit injection. Many examples show fetching data -via `this.store` but do not specifiy how `this.store` arrived as a property on the Route. Also apparent -disclaimers for people visiting the `ember-data` docs that explicit injection of `@ember-data/store` is necessary -and has deviated from past behaviour is likely prudent. +The API docs would need to be overhauld in a few spots. First, we need to remove the [docs](https://guides.emberjs.com/release/applications/dependency-injection/#toc_factory-injections) about Factory injections. Second, we need to detail explicit injection where currently relying on implicit injection. Many examples show fetching data via `this.store` but do not specifiy how `this.store` arrived as a property on the Route. Also apparent disclaimers for people visiting the `ember-data` docs that explicit injection of `@ember-data/store` is necessary and has deviated from past behaviour is likely prudent. The default blueprints when generating a new Ember application will not change. From 520232741b5fb9367ad1df32270b812df94937c2 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Thu, 19 Nov 2020 20:59:29 -0800 Subject: [PATCH 11/14] add pr and issue --- text/0680-implicit-injection-deprecation.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index a4eff90289..ea39786ba9 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -1,7 +1,7 @@ - Start Date: 2020-10-04 - Relevant Team(s): Ember.js -- RFC PR: (after opening the RFC PR, update this with a link to it and update the file name) -- Tracking: (leave this empty) +- RFC PR: https://github.com/emberjs/rfcs/pull/680 +- Tracking: https://github.com/emberjs/rfcs/issues/508 # Implicit Injection Deprecation From 435e17cbe84fc556f0c3c97b7ea37bf50113d96d Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Fri, 4 Dec 2020 16:13:33 -0800 Subject: [PATCH 12/14] Phase 2 will happen in 4.0.0 with owner.inject as a noop --- text/0680-implicit-injection-deprecation.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index ea39786ba9..550919d14f 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -60,22 +60,23 @@ Removing implicit injections is not possible until we first issue deprecations. ### 1. Deprecate implicit injection on target object -Simply deprecating `owner.inject` is not sufficient because it is very difficult to detect if you are relying on implicit injection. As a result, the first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter and setter on the target. Whenever the property is accessed, we will check if the property is explicitly injected. If it isn't, we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will not issue this deprecation and continue assigning the property. +Simply deprecating `owner.inject` is not sufficient because it is very difficult to detect if you are relying on implicit injection. As a result, the first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter and setter on the target. Whenever the property is accessed, we will check if the property is explicitly injected. If it isn't, we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will not issue this deprecation and will continue assigning the property. This deprecation will go `until: 4.0.0`. -To avoid this deprecation, you will need to explicitly add the injected property to the target object. As an example, you can add `@service store` to your route or controller to avert this deprecation resulting from `@ember/data`'s use of `owner.inject`. +To avoid this deprecation, you will need to explicitly add the injected property to the target object. As an example, you can add `@service store` to your route or controller to avert this deprecation resulting from `@ember/data`'s use of `owner.inject`. Users and addons can remove implicit injections as well. -### 2. Deprecate `owner.inject` +Ember.js `v4.0.0` will still be released with `owner.inject`. However, it will not doing anything. Effectively a no-op. -The first phase did not actually deprecate the use of `owner.inject`. As a result, we need to deprecate it's use directly before removing. +### 2. Deprecate `owner.inject`. -For users and or addons, you may be able to remove `owner.inject` to avoid this deprecation. However, for libraries like `@ember/data`, we will not be able to remove until `@ember/data` v4. +The first phase did not actually deprecate the use of `owner.inject`. As a result, we need to deprecate it's use directly iin `v4.0.0` before removing completely in `v5.0.0`. -### 3. Profit! +For users and or addons, you likely already have removed implicit injections like `owner.inject` before upgrading to Ember.js `v4.0.0`. However, for libraries like `@ember/data`, we will look to remove sometime in `v4.0.0` to resolve this new deprecation. -Ember 4.0 will remove the ability to utilize `owner.inject` to inject arbitrary properties on Ember's Framework objects. +### 3. Profit! +Ember.js `v5.0.0` will finally remove the ability to call `owner.inject` to inject arbitrary properties on Ember's Framework objects. -It is important to consider the timeline of these three phases. Each step will consist of a minimum of one release cycle. Many addons and apps will need to make minor and major changes to their codebase. We need to consider this as we move through each phase. +It is important to consider the timeline of these three phases. The first step will consist of a minimum of one release cycle. Many addons and apps will need to make minor and major changes to their codebases before `v4.0.0`. ## How we teach this From 2c8d204d4e7e57cad7ec2f50c660945722a0b93b Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Sat, 5 Dec 2020 22:01:41 -0800 Subject: [PATCH 13/14] add deprecation guides --- text/0680-implicit-injection-deprecation.md | 160 ++++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index 550919d14f..d0b1641ab9 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -84,6 +84,166 @@ The API docs would need to be overhauld in a few spots. First, we need to remove The default blueprints when generating a new Ember application will not change. +### Deprecation Guides + +#### Stage 1 + +Implicit injections are injections that are made by telling Ember to inject a +service (or another type of value) into every instance of a specific type of +object. A common example of this was the `store` property that was injected into +routes and controllers when users installed Ember Data by default. + +```js +export default class ApplicationRoute extends Route { + model() { + return this.store.findQuery('user', 123); + } +} +``` + +Notice how the user can access `this.store` without having declared the store +service using the `@service` decorator. This was accomplished by using the +`owner.inject` API, usually in an initializer: + +```js +export default { + initialize(app) { + app.inject('route', 'store', 'service:store'); + app.inject('controller', 'store', 'service:store'); + } +} +``` + +Implicit injections are difficult to understand, both because it's not obvious +that they exist, or where they come from. + +In general, in order to migrate away from this pattern, you should use an +explicit injection instead of an implicit one. You can do this by using the +`@service` decorator wherever you are using the implicit injection currently. + +Before: + +```js +import { Route } from '@ember/routing/route'; + +export default class ApplicationRoute extends Route { + model() { + return this.store.findQuery('user', 123); + } +} +``` + +After: + +```js +import { Route } from '@ember/routing/route'; +import { inject as service } from '@ember/service'; + +export default class ApplicationRoute extends Route { + @service store; + + model() { + return this.store.findQuery('user', 123); + } +} +``` + +In some cases, you may be using an injected value which is not a service. +Injections of non-service values do not have a direct explicit-injection +equivalent. As such, to migrate away from these, you will have to rewrite the +injection to use services instead. + +Before: + +```js +// app/initializers/logger.js +import EmberObject from '@ember/object'; + +export function initialize(application) { + let Logger = EmberObject.extend({ + log(m) { + console.log(m); + } + }); + + application.register('logger:main', Logger); + application.inject('route', 'logger', 'logger:main'); +} + +export default { + name: 'logger', + initialize: initialize +}; +``` +```js +// app/routes/application.js +export default class ApplicationRoute extends Route { + model() { + this.logger.log('fetching application model...'); + //... + } +} +``` + +After: + +```js +// app/services/logger.js +import Service from '@ember/service'; + +export class Logger extends Service { + log(m) { + console.log(m); + } +} +``` +```js +// app/routes/application.js +import { inject as service } from '@ember/service'; + +export default class ApplicationRoute extends Route { + @service logger; + + model() { + this.logger.log('fetching application model...'); + //... + } +} +``` + +### Stage 2 + +The `owner.inject` API would previously inject a values into every instance of a +particular type of class. For instance, you could inject the `store` service +automatically into every controller and route: + + +```js +export default { + initialize(app) { + app.inject('route', 'store', 'service:store'); + app.inject('controller', 'store', 'service:store'); + } +} +``` + +And in doing so, users could use the value without explicitly declaring it in +that type of object, using `@service`: + +```js +import { Route } from '@ember/routing/route'; + +export default class ApplicationRoute extends Route { + model() { + return this.store.findQuery('user', 123); + } +} +``` + +This ability was deprecated and removed in Ember v4.0.0, so users no longer can +do this, even if you are using the `owner.inject` API. It instead does nothing, +so you can safely remove it without any other changes necessary. + ## Alternatives - Continue with use of `owner.inject` but overhaul docs and recommend explicit injection. From 6f205b413a6c246fb57c320fc2ea5be73d9be143 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Thu, 17 Dec 2020 14:02:44 -0800 Subject: [PATCH 14/14] updates based on feedback --- text/0680-implicit-injection-deprecation.md | 64 ++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/text/0680-implicit-injection-deprecation.md b/text/0680-implicit-injection-deprecation.md index d0b1641ab9..90c0be5856 100644 --- a/text/0680-implicit-injection-deprecation.md +++ b/text/0680-implicit-injection-deprecation.md @@ -60,7 +60,9 @@ Removing implicit injections is not possible until we first issue deprecations. ### 1. Deprecate implicit injection on target object -Simply deprecating `owner.inject` is not sufficient because it is very difficult to detect if you are relying on implicit injection. As a result, the first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter and setter on the target. Whenever the property is accessed, we will check if the property is explicitly injected. If it isn't, we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will not issue this deprecation and will continue assigning the property. This deprecation will go `until: 4.0.0`. +Simply deprecating `owner.inject` is not sufficient because it is very difficult to detect if you are relying on implicit injection. As a result, the first step will be issuing a deprecation on the target factory object that is receiving a property. This will surface the deprecation on user's owned objects including transitively through addons like `ember-data`. This can be accomplished by installing a native getter and setter on the target. + +The _first time_ the property is accessed, either through getting or setting it, we will check if the property is explicitly injected. If it isn't, we will issue a deprecation in DEBUG builds with an until, id and url value to read more about this deprecation. In production, we will not issue this deprecation and will continue assigning the property. This deprecation will go `until: 4.0.0`. To avoid this deprecation, you will need to explicitly add the injected property to the target object. As an example, you can add `@service store` to your route or controller to avert this deprecation resulting from `@ember/data`'s use of `owner.inject`. Users and addons can remove implicit injections as well. @@ -78,6 +80,32 @@ Ember.js `v5.0.0` will finally remove the ability to call `owner.inject` to inje It is important to consider the timeline of these three phases. The first step will consist of a minimum of one release cycle. Many addons and apps will need to make minor and major changes to their codebases before `v4.0.0`. +### Impementation Constraints + +An important implementation detail of stage 1 of this RFC is that while we want +to issue a deprecation when the user relies on the implicit injection, we also +_must_ ensure that the implicit injection still "wins" and is assigned to the +value on the class, clobbering the explicit injection, for the time being. This +is the current behavior, and changing it, even in development builds, would be a +breaking change. + +The proposed way to implement this is when an object with implicit injections is +created, for each implicit injection: + +1. If the property already contains the value, do nothing as it has been setup + eagerly by the user +2. If the property is a service injection, check that it is the correct service + injection and if it is, do nothing and assign the value like normal +3. If the property is a native getter/setter, wrap the native getter/setter and + the first time the value is accessed, check to see if it is the injected + value. If it is, do nothing, otherwise log the deprecation. +4. If the property is not defined on the object or its prototype, install a + getter/setter pair which log the deprecation the first time they are + activated. + +This should preserve the existing semantics while logging when the user is +accessing a value that was injected implicitly. + ## How we teach this The API docs would need to be overhauld in a few spots. First, we need to remove the [docs](https://guides.emberjs.com/release/applications/dependency-injection/#toc_factory-injections) about Factory injections. Second, we need to detail explicit injection where currently relying on implicit injection. Many examples show fetching data via `this.store` but do not specifiy how `this.store` arrived as a property on the Route. Also apparent disclaimers for people visiting the `ember-data` docs that explicit injection of `@ember-data/store` is necessary and has deviated from past behaviour is likely prudent. @@ -211,6 +239,40 @@ export default class ApplicationRoute extends Route { } ``` +In cases where it is not possible to convert a custom injection type into a +service, the value can be accessed by looking it up directly on the container +instead using the [lookup](https://api.emberjs.com/ember/3.22/classes/ApplicationInstance/methods/lookup?anchor=lookup) +method: + +```js +// app/routes/application.js +import { getOwner } from '@ember/application'; +import { inject as service } from '@ember/service'; + +export default class ApplicationRoute extends Route { + get logger() { + if (this._logger === undefined) { + this._logger = getOwner(this).lookup('logger:main'); + } + + return this._logger; + } + + set logger(value) { + this._logger = value; + } + + model() { + this.logger.log('fetching application model...'); + //... + } +} +``` + +You should always include a setter until the implicit injection is removed, +since the container will still attempt to pass it into the class on creation, +and it will cause errors if it attempts to overwrite a value without a setter. + ### Stage 2 The `owner.inject` API would previously inject a values into every instance of a