From e0a05c2293fa4a9fe1fdb16dadd5a947cfcb5c0e Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Tue, 19 Feb 2019 17:12:08 -0800 Subject: [PATCH 1/8] Add Classic Class Owner Tunnel RFC --- text/0000-classic-class-owner-tunnel.md | 205 ++++++++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 text/0000-classic-class-owner-tunnel.md diff --git a/text/0000-classic-class-owner-tunnel.md b/text/0000-classic-class-owner-tunnel.md new file mode 100644 index 0000000000..57516036b3 --- /dev/null +++ b/text/0000-classic-class-owner-tunnel.md @@ -0,0 +1,205 @@ +- Start Date: 2019-02-19 +- Relevant Team(s): Ember.js, Learning +- RFC PR: +- Tracking: + +# Classic Class Owner Tunnel 🕳 + +## Summary + +Make `getOwner` and explicit injections work in classic class constructors. + +## Terminology + +- _Explicit injections_ are injections which are defined in the class body using + the `inject` APIs: + + ```js + import Component from '@ember/component'; + import { inject as service } from '@ember/service'; + + export default Component.extend({ + store: service(), + }); + ``` + + The are _explicit_ because they don't require any knowledge of the system to + outside of the class itself to know they exist. + +- _Implicit injections_ are injections which are defined using the container + APIs directly, often in initializers: + + ```js + import Application from '@ember/application'; + + Application.initializer({ + name: 'inject-session', + + initialize() { + // Inject the session service onto all factories of the type 'controller' + // with the name 'session' + App.inject('controller', 'session', 'service:session'); + }, + }); + ``` + + They are implicit because they require knowledge of the context + of the class to know whether or not they exist, simply looking at the class + body (without looking at method logic) will not hint at their existence. The + canonical example here is the Ember Data `store`, which is implicitly injected + into all routes and controllers. + +## Motivation + +The [Native Class Constructor Update][1] RFC changed the way that classic +classes were constructed, and one major side effect of this was that it made +injections and the container inaccessible during the `constructor` of classes +that extend from `EmberObject`. This was a necessary change for other reasons, +but the lack of injections means that we still have to use `init` for various +use cases in several classes, including: + +[1]: https://github.com/emberjs/rfcs/blob/master/text/0337-native-class-constructor-update.md + +- `Service` +- `Route` +- `Controller` + +These classes would otherwise be _completely free_ of any of the trappings of +the classic object model. As the Octane striketeam has begun updating the guides +for these, the fact that users must learn to use `init` with them has become a +pain point in the new guides. This is _especially_ painful given that +`GlimmerComponent` does _not_ have an `init` hook, meaning we can't teach users +to just use `init` everywhere either. Instead, users must learn that they have +to use one or the other depending on what type of class they're using, even in +completely new Ember Octane apps. + +By tunneling the owner through to the root constructor, we can unify our +documentation and best practices around one recommendation. New Ember Octane +applications will be able to completely forgo `init`, and never learn about it +in the first place. Existing Ember apps will still have cases where it is +necessary (classic components, utility classes), but this is no worse a status +quo than where we are currently - some classes require `constructor`, and some +require `init`. + +## Detailed design + +The "tunnel" itself is fairly simple. As described in the Constructor Update +RFC, this is how the `create` method on classes works currently: + +```js +class EmberObject { + constructor() { + // ..class setup things + } + + static create(args) { + let instance = new this(); + + Object.assign(instance, args); + instance.init(); + + return instance; + } +} +``` + +We would update this to the following: + +```js +class EmberObject { + constructor(owner) { + setOwner(this, owner); + // ..class setup things + } + + static create(args) { + let owner = args ? getOwner(args) : undefined; + let instance = new this(owner); + + Object.assign(instance, args); + instance.init(); + + return instance; + } +} +``` + +Now, when any subclass's constructor code is run it will have the owner +available, which in turn makes all _explicit_ injections work (they use +`getOwner` under the hood). + +However, _implicit_ injections will still only be available during `init`, +because they are passed in and assigned as `args`. This RFC proposes that rather +than attempting to fix implicit injections, we create development-mode +assertions for them which throw if a user attempts to use them during the +`constructor`, before they are assigned. This will give a helpful error message +that instructs them to add the injection explicitly (ideally), or to use `init`. + +### Backwards Compatibility + +This change is backwards compatible, so existing applications will not be +affected. Some users may feel like there is some churn, especially TypeScript +users (sorry folks!) + +These changes will also be backported to at least: + +- lts-3.8 +- lts-3.4 + +via the `ember-native-class-polyfill`, which currently supports polyfilling to +Ember@3.4. If possible, that range will be extended to the last v2 LTS versions. + +## How we teach this + +This change would take some burden off of the guides for _new_ Ember users, +post-Octane, since it would simplify them. New documentation should only refer +to `constructor` when talking about native class syntax, and should guide users +toward using `constructor` over `init`. + +For existing apps and upgrade documentation, the distinction needs to be made +clear about the two types of classes that _should_ still use `init`: + +- Classic Components +- Utility Classes (e.g. user defined classes that extend `EmberObject`) + +These two require `init` if users need access to component args or create args, +respectively. + +The main guides will recommend that users refactor these classes entirely rather +than convert them to native classes. Classic components should become Glimmer +components, and utility classes should be refactor _away_ from extending +`EmberObject`. + +Supplemental guides should also be written to cover the nuances of using native +class syntax with either of these, including when and where to use `init`. +Additionally, lint rules should be made if possible that enforce that the user +is using the correct method. These lint rules may need some hinting in the form +of a decorator or other "trigger" that let's them know what type of class a +given class is: + +```js +// This tell the linter that this is a classic component that needs to be upgraded +@todo('octane-upgrade') +export default class FooComponent extends ClassicComponent { + // this throws an error because of the annotation, but will be fine once the + // annotation is removed. + constructor() {} +} +``` + +## Drawbacks + +- More churn in the ecosystem, early adopters of classes already switched from + `constructor` -> `init`, switching back would be painful. +- Where to use `init` and where to use `constructor` may be a bit less clear + after. This was already a concern with `GlimmerComponent`, but it may be more + problematic if there are more exceptions. + +## Alternatives + +- Add an `init` hook to `GlimmerComponent` to unify it with the classic classes. + This could be confusing to users of `GlimmerComponent` (why do injections + work in GC but not any other class constructor? Why does GC have `init` _and_ + `constructor`?) +- Keep using `init` for classic classes for the indefinite future, and teach + around. From 491414411b6a78299e0fcc619a5bbef287b66207 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Wed, 20 Feb 2019 07:56:46 -0800 Subject: [PATCH 2/8] Add PR link --- text/0000-classic-class-owner-tunnel.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-classic-class-owner-tunnel.md b/text/0000-classic-class-owner-tunnel.md index 57516036b3..8d35344744 100644 --- a/text/0000-classic-class-owner-tunnel.md +++ b/text/0000-classic-class-owner-tunnel.md @@ -1,6 +1,6 @@ - Start Date: 2019-02-19 - Relevant Team(s): Ember.js, Learning -- RFC PR: +- RFC PR: https://github.com/emberjs/rfcs/pull/451 - Tracking: # Classic Class Owner Tunnel 🕳 From 4c6f0f01a14ad65482d8b5e443050196ea7e7cef Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Wed, 20 Feb 2019 08:28:39 -0800 Subject: [PATCH 3/8] add deprecation motivation --- text/0000-classic-class-owner-tunnel.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/text/0000-classic-class-owner-tunnel.md b/text/0000-classic-class-owner-tunnel.md index 8d35344744..49da18d2b6 100644 --- a/text/0000-classic-class-owner-tunnel.md +++ b/text/0000-classic-class-owner-tunnel.md @@ -81,6 +81,13 @@ necessary (classic components, utility classes), but this is no worse a status quo than where we are currently - some classes require `constructor`, and some require `init`. +Finally, if we do this change, we could lint users to a place where they are not +using _any_ EmberObject APIs at all for any of these constructs. This means that +in the future, we _could_ deprecate them extending from `EmberObject` at all, +without forcing users onto new import paths. This deprecation is outside of the +scope of this RFC, but it sets us up to move forward with decoupling Ember from +the classic object model in the future. + ## Detailed design The "tunnel" itself is fairly simple. As described in the Constructor Update From fd27fc2c81266bff002a648f3b8e3c12e093e910 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Thu, 14 Mar 2019 22:31:00 -0700 Subject: [PATCH 4/8] Update name and motivation --- text/0000-classic-class-owner-tunnel.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/text/0000-classic-class-owner-tunnel.md b/text/0000-classic-class-owner-tunnel.md index 49da18d2b6..f6a41b96dc 100644 --- a/text/0000-classic-class-owner-tunnel.md +++ b/text/0000-classic-class-owner-tunnel.md @@ -3,11 +3,12 @@ - RFC PR: https://github.com/emberjs/rfcs/pull/451 - Tracking: -# Classic Class Owner Tunnel 🕳 +# Injection Parameter Normalization ## Summary -Make `getOwner` and explicit injections work in classic class constructors. +Normalize on passing the `owner` as the first parameter to the constructor of +classes that are created by the container. ## Terminology From f3525d62e3ec197738331ce685bbdb5ae820b116 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Wed, 27 Mar 2019 13:00:59 -0700 Subject: [PATCH 5/8] update to target only framework classes --- ...0000-injection-parameter-normalization.md} | 136 +++++++++++------- 1 file changed, 81 insertions(+), 55 deletions(-) rename text/{0000-classic-class-owner-tunnel.md => 0000-injection-parameter-normalization.md} (55%) diff --git a/text/0000-classic-class-owner-tunnel.md b/text/0000-injection-parameter-normalization.md similarity index 55% rename from text/0000-classic-class-owner-tunnel.md rename to text/0000-injection-parameter-normalization.md index f6a41b96dc..47673e10a4 100644 --- a/text/0000-classic-class-owner-tunnel.md +++ b/text/0000-injection-parameter-normalization.md @@ -7,8 +7,15 @@ ## Summary -Normalize on passing the `owner` as the first parameter to the constructor of -classes that are created by the container. +Normalize on passing the `owner` as the first parameter to the constructor for +Ember's built in framework classes that are constructed by the container: + +- `GlimmerComponent` +- `EmberComponent` +- `Service` +- `Route` +- `Controller` +- `Helper` ## Terminology @@ -52,45 +59,78 @@ classes that are created by the container. ## Motivation -The [Native Class Constructor Update][1] RFC changed the way that classic -classes were constructed, and one major side effect of this was that it made -injections and the container inaccessible during the `constructor` of classes -that extend from `EmberObject`. This was a necessary change for other reasons, -but the lack of injections means that we still have to use `init` for various -use cases in several classes, including: - -[1]: https://github.com/emberjs/rfcs/blob/master/text/0337-native-class-constructor-update.md - +The introduction of native class syntax in Ember has recently exposed some of +the inner-workings and expectations of Ember's Dependency Injection (DI) system. +Specifically, it is now possible to write code that can run _before_ +dependencies are injected in some base classes, such as Services and +Controllers. Currently, users must use the `init` hook in these classes if they +wish to run setup code that accesses injections, but this is somewhat confusing +since `init` has historically been taught as the same as the `constructor` in +native classes. + +Glimmer components made the decision to break from this pattern, and instead +pass the DI Owner as the first parameter to the constructor. They then set it +using `setOwner` in the base class, making injections available during the +constructor, and to class field initializers. + +So far this has worked pretty well in practice: + +- Glimmer components have just 2 lifecycle hooks, which makes them simpler to + understand and learn about. +- We don't have to teach the differences between `constructor` and `init`, when + to use one or the other, and debugging issues when the two have mixed usage + throughout the class hierarchy +- We don't have to worry about explaining the timings/lifecycle of the container + and the way it constructs classes in order to explain why these are separate. + +This RFC seeks to normalize this contract for all _Ember base classes_ - that +is, framework classes that are provided by Ember and instantiated by the +container, including: + +- `GlimmerComponent` +- `EmberComponent` - `Service` - `Route` - `Controller` +- `Helper` + +This RFC does **_not_** aim to provide a single contract for _all_ classes +registered on the container, in perpetuity. This would lock us into a tight +coupling between the container and constructors for objects that are registered, +and wouldn't provide much flexibility in the future. -These classes would otherwise be _completely free_ of any of the trappings of -the classic object model. As the Octane striketeam has begun updating the guides -for these, the fact that users must learn to use `init` with them has become a -pain point in the new guides. This is _especially_ painful given that -`GlimmerComponent` does _not_ have an `init` hook, meaning we can't teach users -to just use `init` everywhere either. Instead, users must learn that they have -to use one or the other depending on what type of class they're using, even in -completely new Ember Octane apps. - -By tunneling the owner through to the root constructor, we can unify our -documentation and best practices around one recommendation. New Ember Octane -applications will be able to completely forgo `init`, and never learn about it -in the first place. Existing Ember apps will still have cases where it is -necessary (classic components, utility classes), but this is no worse a status -quo than where we are currently - some classes require `constructor`, and some -require `init`. - -Finally, if we do this change, we could lint users to a place where they are not -using _any_ EmberObject APIs at all for any of these constructs. This means that -in the future, we _could_ deprecate them extending from `EmberObject` at all, -without forcing users onto new import paths. This deprecation is outside of the -scope of this RFC, but it sets us up to move forward with decoupling Ember from -the classic object model in the future. +Instead, we believe we should continue exploring APIs for generalizing the way +DI is configured for a given base class. It could be done via custom managers, +or via decoration, like the [Injection Hook Normalization +RFC](https://github.com/emberjs/rfcs/pull/467). When these APIs are fully +rationalized and accepted, we'll update Ember's base classes to use them to +specify the owner injection like any other user class could. ## Detailed design +This RFC has 2 major parts: + +1. The contract that we'll uphold for dependency injection in _Ember base + classes_. +2. The implementation of that contract for existing base classes (the tunnel). + +### Dependency Injection Contract + +For all Ember base classes created by the container, such as `GlimmerComponent`, +`Service`, `Controller`, etc. we will: + +1. Pass the owner as the first parameter when constructing the class. +2. Set the owner with `setOwner` in the base class constructor. + +This will make explicit injections available during the `constructor` method of +the class, and for access by class field initializers. + +This contract _only_ applies to Ember base classes and framework objects, and +classes that extend `EmberObject`. It does _not_ necessarily apply to arbitrary +classes that are created and registered in the container. + +### Implementation + The "tunnel" itself is fairly simple. As described in the Constructor Update RFC, this is how the `create` method on classes works currently: @@ -146,10 +186,7 @@ that instructs them to add the injection explicitly (ideally), or to use `init`. ### Backwards Compatibility This change is backwards compatible, so existing applications will not be -affected. Some users may feel like there is some churn, especially TypeScript -users (sorry folks!) - -These changes will also be backported to at least: +affected. These changes will also be backported to at least: - lts-3.8 - lts-3.4 @@ -178,22 +215,11 @@ than convert them to native classes. Classic components should become Glimmer components, and utility classes should be refactor _away_ from extending `EmberObject`. -Supplemental guides should also be written to cover the nuances of using native -class syntax with either of these, including when and where to use `init`. -Additionally, lint rules should be made if possible that enforce that the user -is using the correct method. These lint rules may need some hinting in the form -of a decorator or other "trigger" that let's them know what type of class a -given class is: - -```js -// This tell the linter that this is a classic component that needs to be upgraded -@todo('octane-upgrade') -export default class FooComponent extends ClassicComponent { - // this throws an error because of the annotation, but will be fine once the - // annotation is removed. - constructor() {} -} -``` +The [`@classic`](https://github.com/emberjs/rfcs/pull/468) decorator will also +provide a way to guide users toward the correct usage, based on whether they are +in "classic" mode or "octane" mode. We will be able to provide linting and +warnings/assertions to prevent users from accidentally using `init` when they +should have used `constructor`, and vice-versa. ## Drawbacks @@ -210,4 +236,4 @@ export default class FooComponent extends ClassicComponent { work in GC but not any other class constructor? Why does GC have `init` _and_ `constructor`?) - Keep using `init` for classic classes for the indefinite future, and teach - around. + around it. From 74456a4cb47b35151133b8dfefd4d46a5f987f94 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Fri, 5 Apr 2019 10:38:04 -0700 Subject: [PATCH 6/8] Apply suggestions from code review Co-Authored-By: pzuraq --- text/0000-injection-parameter-normalization.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/text/0000-injection-parameter-normalization.md b/text/0000-injection-parameter-normalization.md index 47673e10a4..c694f1be93 100644 --- a/text/0000-injection-parameter-normalization.md +++ b/text/0000-injection-parameter-normalization.md @@ -8,7 +8,7 @@ ## Summary Normalize on passing the `owner` as the first parameter to the constructor for -Ember's built in framework classes that are constructed by the container: +the following built in framework classes: - `GlimmerComponent` - `EmberComponent` @@ -70,7 +70,7 @@ native classes. Glimmer components made the decision to break from this pattern, and instead pass the DI Owner as the first parameter to the constructor. They then set it -using `setOwner` in the base class, making injections available during the +using `setOwner` in the base class, making explicit injections available during the constructor, and to class field initializers. So far this has worked pretty well in practice: @@ -84,8 +84,7 @@ So far this has worked pretty well in practice: and the way it constructs classes in order to explain why these are separate. This RFC seeks to normalize this contract for all _Ember base classes_ - that -is, framework classes that are provided by Ember and instantiated by the -container, including: +is, framework classes that are provided by Ember: - `GlimmerComponent` - `EmberComponent` @@ -126,17 +125,18 @@ This will make explicit injections available during the `constructor` method of the class, and for access by class field initializers. This contract _only_ applies to Ember base classes and framework objects, and -classes that extend `EmberObject`. It does _not_ necessarily apply to arbitrary +classes that extend `EmberObject`. It does _not_ apply to arbitrary classes that are created and registered in the container. ### Implementation The "tunnel" itself is fairly simple. As described in the Constructor Update -RFC, this is how the `create` method on classes works currently: +RFC, this is how the `create` method on framework classes works currently: ```js -class EmberObject { +class Service extends EmberObject { constructor() { + super(); // ..class setup things } @@ -154,8 +154,9 @@ class EmberObject { We would update this to the following: ```js -class EmberObject { +class Service extends EmberObject { constructor(owner) { + super(); setOwner(this, owner); // ..class setup things } From d2d078a5d91215f6ff353617f509e20cb8c2532a Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Sat, 6 Apr 2019 10:29:33 -0700 Subject: [PATCH 7/8] add data classes --- text/0000-injection-parameter-normalization.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/text/0000-injection-parameter-normalization.md b/text/0000-injection-parameter-normalization.md index c694f1be93..e2515e8572 100644 --- a/text/0000-injection-parameter-normalization.md +++ b/text/0000-injection-parameter-normalization.md @@ -1,5 +1,5 @@ - Start Date: 2019-02-19 -- Relevant Team(s): Ember.js, Learning +- Relevant Team(s): Ember.js, Ember Data, Learning - RFC PR: https://github.com/emberjs/rfcs/pull/451 - Tracking: @@ -17,6 +17,13 @@ the following built in framework classes: - `Controller` - `Helper` +Along with the following Ember Data classes: + +- `Model` +- `Adapter` +- `Serializer` +- `Transform` + ## Terminology - _Explicit injections_ are injections which are defined in the class body using @@ -93,6 +100,13 @@ is, framework classes that are provided by Ember: - `Controller` - `Helper` +Along with framework clases provided by Ember Data: + +- `Model` +- `Adapter` +- `Serializer` +- `Transform` + This RFC does **_not_** aim to provide a single contract for _all_ classes registered on the container, in perpetuity. This would lock us into a tight coupling between the container and constructors for objects that are registered, From 7fdf3be9977522c959ebfcb1b329f500c195eb91 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Fri, 12 Apr 2019 14:50:07 -0400 Subject: [PATCH 8/8] Update filename to include PR number. --- ...normalization.md => 0451-injection-parameter-normalization.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{0000-injection-parameter-normalization.md => 0451-injection-parameter-normalization.md} (100%) diff --git a/text/0000-injection-parameter-normalization.md b/text/0451-injection-parameter-normalization.md similarity index 100% rename from text/0000-injection-parameter-normalization.md rename to text/0451-injection-parameter-normalization.md