From 412747417712f9d16a333dd1d05c9692c349b060 Mon Sep 17 00:00:00 2001 From: Matthew Beale Date: Fri, 1 Jul 2016 16:27:55 -0400 Subject: [PATCH 1/4] Add factoryFor RFC --- text/0000-factoryFor.md | 280 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 text/0000-factoryFor.md diff --git a/text/0000-factoryFor.md b/text/0000-factoryFor.md new file mode 100644 index 0000000000..251106fb2e --- /dev/null +++ b/text/0000-factoryFor.md @@ -0,0 +1,280 @@ +- Start Date: 2016-06-11 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Summary + +With the goal of making significant performance improvements and of adding +public API to support use cases long-served by a private API, a new API of +`factoryFor` will be added to `ApplicationInstance` instances. + +# Motivation + +Ember's dependency injection container has long supported fetching a factory +that will be created with any injections present. Using the private API that +provided this support would allow an instance of the factory to be created +with initial values passed via `create`. For example: + +```js +import Ember from 'ember'; +const { Component, getOwner } = Ember; + +export default Component.extend( + init() { + this._super(...arguments); + let Factory = getOwner(this)._lookupFactory('logger:main'); + this.logger = Factory.create({ level: 'low' }); + } +}); +``` + +In this API, the `Factory` is actually a subclass the main logger class. In +`_lookupFactory`, there are minimum two class `extend` calls: + +* `MyClass = Ember.Object.extend(` +* `MyFactoryWithInjections = MyClass.extend(` +* `MyFactoryWithInjections.create(` + +The middle extend only serves to implement dependency injection- merging the +properties passed to `create` with the injected ones. The double extend +of classes in Ember takes a toll on performance booting an app. This +poor design has kept the factory lookup API private. + +Additionally the middle extend is a cache, and cleared between tests or +application instances. For example: + +``` + +-------------------------------+ + | | + | /app/models/foo.js | + | | + +-------------------------------+ + | + first test run | nth test run + +----------------+---------------+ + | | + v v + +---------------------+ +--------------------+ + |resolve('model:foo') | === |resolve('model:foo')| + +---------------------+ +--------------------+ + | | + | | + v v + + extend(injections) extend(injections) + + | | + | | + | | + v v ++--------------------------+ +---------------------------+ +|lookupFactory('model:foo')| !== |lookupFactory('model:foo') | ++--------------------------+ +---------------------------+ +``` + +However despite these issues being able to instantiate a factory with initial properties is useful for +a number of scenarios and is a common practice +in a number of important addons. + +* [ember-cart](https://github.com/DockYard/ember-cart) uses the functionality to create model objects without + tying them to the store [example a](https://github.com/DockYard/ember-cart/blob/c01eb22eaf2e97f8c80481c3174d4be917e476a9/tests/dummy/app/controllers/application.js#L16), + [example b](https://github.com/DockYard/ember-cart/blob/c01eb22eaf2e97f8c80481c3174d4be917e476a9/tests/dummy/app/models/dog.js#L11) +* Ember-Data's [`modelFactoryFor`](https://github.com/emberjs/data/blob/54ea432b1cbb0d1231d9a0454b09d3b3a0bc2533/addon/-private/system/store.js#L1868) + +Many use-cases can be handled with a convention of calling a `setup` method +on object instances after their creation by the container. However this convention +would be at odds with common OO patterns (where you use the constructor to +setup an instance) and performance best practices (which prefer to see +properties defined on an object during its construction). + +# Detailed design + +This feature will be added in three steps. + +1. Introduce `ApplicationInstance#factoryFor` in Ember 2.8 +2. Deprecate `ApplicationInstance#_lookupFactory` in Ember 2.8, remove in 2.9 +3. Release a polyfill addon for this feature + +#### Introduce `ApplicationInstance#factoryFor` in Ember 2.8 + +A new API will be introduced. This API will return both the original base +class resisted (or resolved) by the container, and will also return a function +to generate a dependency-injected instance. For example: + +```js +import Ember from 'ember'; +const { Component, getOwner } = Ember; + +export default Component.extend( + init() { + this._super(...arguments); + let factory = getOwner(this).factoryFor('logger:main'); + this.logger = factory.create({ level: 'low' }); + } +}); +``` + +Unlike `_lookupFactory`, `factoryFor` will not return an extended class with +DI applied. Instead it will return a factory object with two properties: + +```js +// factoryFor returns: +let { + + // a function taking an argument of initial properties passed to the object + // and returning an instance + create, + + // The class registered into (or resolved by) the container + class + +} = owner.factoryFor('type:name'); +``` + +This API should meet two requirements of the use-cases described in +"Motivation": + +* The implementation of `create` can diverge away from double extend. The + class of an object instantiated via `_lookupFactory(name).create()` + and `factoryFor(name).create()` may not be the same, even given the + same factory being registered into the container. +* The presence of `class` will make it easy to identify the base class of the + factory at runtime. + +For example today's `_lookupFactory` creates an inheritance structure like +the following: + +``` + Current: + +-------------------------------+ + | | + | /app/models/foo.js | + | | + +-------------------------------+ + | + | + | + v + +--------------------+ + | Class[model/Foo] | + +--------------------+ + | + | + | + first test run | nth test run + +-----------+----------+ + | | + | | + | | + v v ++--------------------+ +--------------------+ +| subclass of | | subclass of | +| Class[model/Foo] | | Class[model/Foo] | ++--------------------+ +--------------------+ +``` + +This means, between test runs 2 instances of `model:foo` will have a common +shared ancestor the grandparent `Class[model/Foo]`. + +We propose is to remove the intermediate subclass and instead have a generic +factory object, which holds the injections and allows for injected instances +to be created. The resulting object graph would look something like this: +(between test runs 2 instance of `model:foo` will have a shared ancestor +(parent) `Class[model/Foo]`. + +``` + Proposed: + +-------------------------------+ + | | + | /app/models/foo.js | + | | + +-------------------------------+ + | + | + | + v + +--------------------+ + | Class[model/Foo] | + +--------------------+ + | + | + | + first test run | nth test run + +----------+-----------+ + | | + | | + | | + v v ++--------------------+ +--------------------+ +| Factory of | | Factory of | +| Class[model/Foo] | | Class[model/Foo] | ++--------------------+ +--------------------+ +``` + +This results in `instance` direct `parent` being shared between test runs. More +specially this means any instance state stored on the `parent` will leak +between tests. + +#### Deprecate `ApplicationInstance#_lookupFactory` in Ember 2.8, remove in 2.9 + +In 2.8 (an LTS release) `_lookupFactory` will be deprecated with a message +suggesting a migration to the new API. In 2.9 the API will be removed. + +#### Release a polyfill addon for this feature + +A polyfill addon, similar to [ember-getowner-polyfill](https://github.com/rwjblue/ember-getowner-polyfill) +will be released for this feature. This polyfill will provide the `factoryFor` +API going back to at least 2.4, provide the API and silence the deprecation +in 2.8, and be a no-op in 2.9+ where Ember provides the `factoryFor` API. + +# How We Teach This + +This feature should be introduced along side `lookup` in the +[relevant guide](https://guides.emberjs.com/v2.6.0/applications/dependency-injection/). +The return value of `factoryFor` should be taught as a POJO and not as +an extended class. + +# Drawbacks + +The main drawback to this solution is the removal of double extend. Double +extend is a performance troll, however it also means if a single class is registered +multiple times each `_lookupFactory` returns a unique factory. It is plausible +that some use-case relying on this behavior would get trolled in the migration +to `factoryFor`, however it is unlikely. + +For example these cases where state is stored on the factory would no +longer be viable: + +- ember-model + - https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L404 + - https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L457 + - https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L723-L725 +- ember-data + - As far as I can tell, the big issues have been resolved! + - if attrs change between test runs (seems very unlikely) then https://github.com/emberjs/data/blob/387630db5e7daec6aac7ef8c6172358a3bd6394c/addon/-private/system/model/attr.js#L57 would be affected +- people using: + - `lookupFactory(x).reopen` / `reopenClass` at runtime (or test time to monkey patch code) + - `lookupFactory(x).something = value` + +# Alternatives + +No other designs have been seriously considered. + +We have considered the possibility that removing `_lookupFactory` in 2.9 +(something LTS technically permits) would be too aggressive for the +community of addons. Providing a polyfill is part of the strategy to handle +this change. + +However if the removal still proves too difficult, an alternative strategy +is possible: + +* Add `factoryFor` in 2.8 +* Deprecate `_lookupFactory` in 2.8, remove it in 3.0 +* Introduce a polyfill add for `factoryFor` valid w/ 2.4-3.0 +* In 2.9, migrate the implementation of `_lookupFactory` to be based on + `factoryFor`. There may be some slight behavior change, or performance + regression when using this API. + +# Unresolved questions + +Are there any use-cases for the double extend not considered? From e7c46ba098838476491c0b86b2772922bf9fb2f2 Mon Sep 17 00:00:00 2001 From: Matthew Beale Date: Mon, 11 Jul 2016 11:02:08 -0400 Subject: [PATCH 2/4] Edits to factoryFor RFC --- text/0000-factoryFor.md | 250 +++++++++++++++++++++++++++++++++------- 1 file changed, 211 insertions(+), 39 deletions(-) diff --git a/text/0000-factoryFor.md b/text/0000-factoryFor.md index 251106fb2e..9533fe21c0 100644 --- a/text/0000-factoryFor.md +++ b/text/0000-factoryFor.md @@ -15,6 +15,15 @@ that will be created with any injections present. Using the private API that provided this support would allow an instance of the factory to be created with initial values passed via `create`. For example: +```js +// app/logger/main.js +import Ember from 'ember'; + +export default Ember.Logger.extend({ + someService: Ember.inject.service() +}); +``` + ```js import Ember from 'ember'; const { Component, getOwner } = Ember; @@ -28,20 +37,28 @@ export default Component.extend( }); ``` -In this API, the `Factory` is actually a subclass the main logger class. In -`_lookupFactory`, there are minimum two class `extend` calls: +In this API, the `Factory` is actually a subclass the original main logger +class. When `_lookupFactory` is called, an additional `extend` takes place +to add any injections (such as `someService` above). The class/object setup +looks like this: + +* In the module: `MyClass = Ember.Object.extend(` +* In `_lookupFactory`: `MyFactoryWithInjections = MyClass.extend(` +* And when used: `MyFactoryWithInjections.create(` -* `MyClass = Ember.Object.extend(` -* `MyFactoryWithInjections = MyClass.extend(` -* `MyFactoryWithInjections.create(` +The second call to `extend` implements Ember's owner/DI +framework and permits `someService` to be resolved later. The "owner" object +is merged into the new `MyFactoryWithInjections` class along with any +registered injections. -The middle extend only serves to implement dependency injection- merging the -properties passed to `create` with the injected ones. The double extend -of classes in Ember takes a toll on performance booting an app. This -poor design has kept the factory lookup API private. +This "double extend" (once at define time, once at `_lookupFactory` time) +takes a toll on performance booting an app. This design flaw has motivated +a desire to keep `_lookupFactory` private. -Additionally the middle extend is a cache, and cleared between tests or -application instances. For example: +The `MyFactoryWithInjections` class also features as a cache. Because it is +attached to the owner/container, it is cleared between test runs or +application instances. To illustrate, this flow-chart shows how +`MyFactoryWithInjections` diverges between tests: ``` +-------------------------------+ @@ -72,20 +89,16 @@ application instances. For example: +--------------------------+ +---------------------------+ ``` -However despite these issues being able to instantiate a factory with initial properties is useful for -a number of scenarios and is a common practice -in a number of important addons. +Despite the design flaws in this API, it does fill a meaningful role in +Ember's DI solution. Use of the private API is common. Some examples: * [ember-cart](https://github.com/DockYard/ember-cart) uses the functionality to create model objects without tying them to the store [example a](https://github.com/DockYard/ember-cart/blob/c01eb22eaf2e97f8c80481c3174d4be917e476a9/tests/dummy/app/controllers/application.js#L16), [example b](https://github.com/DockYard/ember-cart/blob/c01eb22eaf2e97f8c80481c3174d4be917e476a9/tests/dummy/app/models/dog.js#L11) * Ember-Data's [`modelFactoryFor`](https://github.com/emberjs/data/blob/54ea432b1cbb0d1231d9a0454b09d3b3a0bc2533/addon/-private/system/store.js#L1868) -Many use-cases can be handled with a convention of calling a `setup` method -on object instances after their creation by the container. However this convention -would be at odds with common OO patterns (where you use the constructor to -setup an instance) and performance best practices (which prefer to see -properties defined on an object during its construction). +The goal of this RFC is to create a public API for fetching factories with +better performance characteristics than `_lookupFactory`. # Detailed design @@ -95,10 +108,10 @@ This feature will be added in three steps. 2. Deprecate `ApplicationInstance#_lookupFactory` in Ember 2.8, remove in 2.9 3. Release a polyfill addon for this feature -#### Introduce `ApplicationInstance#factoryFor` in Ember 2.8 +#### Step #1: Introduce `ApplicationInstance#factoryFor` in Ember 2.8 A new API will be introduced. This API will return both the original base -class resisted (or resolved) by the container, and will also return a function +class registed into or resolved by the container, and will also return a function to generate a dependency-injected instance. For example: ```js @@ -134,10 +147,12 @@ let { This API should meet two requirements of the use-cases described in "Motivation": -* The implementation of `create` can diverge away from double extend. The +* Because `factoryFor` only returns a `create` method and reference to the + original class, its internal implementation can diverge away from the + "double extend". A side-effect of this is that the class of an object instantiated via `_lookupFactory(name).create()` - and `factoryFor(name).create()` may not be the same, even given the - same factory being registered into the container. + and `factoryFor(name).create()` may not be the same, given the + same original factory. * The presence of `class` will make it easy to identify the base class of the factory at runtime. @@ -176,10 +191,11 @@ the following: This means, between test runs 2 instances of `model:foo` will have a common shared ancestor the grandparent `Class[model/Foo]`. -We propose is to remove the intermediate subclass and instead have a generic -factory object, which holds the injections and allows for injected instances +This implementation of `factoryFor` proposes to remove the intermediate +subclass and instead have a generic +factory object which holds the injections and allows for injected instances to be created. The resulting object graph would look something like this: -(between test runs 2 instance of `model:foo` will have a shared ancestor +(between test runs the 2 instances of `model:foo` will have a shared ancestor (parent) `Class[model/Foo]`. ``` @@ -211,16 +227,38 @@ to be created. The resulting object graph would look something like this: +--------------------+ +--------------------+ ``` -This results in `instance` direct `parent` being shared between test runs. More -specially this means any instance state stored on the `parent` will leak -between tests. +With `factoryFor` instances of `model:foo` will share a common constructor. +Any state stored on the constructor would of course leak between the tests. + +An example implementation of `factoryFor` can be reviewed [on this GitHub +comment](https://github.com/emberjs/rfcs/issues/125#issuecomment-193827658). + +##### Implications for `owner.register` -#### Deprecate `ApplicationInstance#_lookupFactory` in Ember 2.8, remove in 2.9 +Currently, factories registered into Ember's DI system are required to +provide an `extend` method. Removing support for extend-based DI in `_lookupFactory` +will permit factories without `extend` to be registered. Instead factories +must only provide a `create` method. For example: + +```js +let factory = { + create(options={}) { + /* Some implementation of `create` */ + return Object.create({}); + } +}; +owner.register('my-type:a-factory', factory); +let factoryWithDI = owner.factoryFor('my-type:a-factory'); + +factoryWithDI.class === factory; +``` + +#### Step #2: Deprecate `ApplicationInstance#_lookupFactory` in Ember 2.8, remove in 2.9 In 2.8 (an LTS release) `_lookupFactory` will be deprecated with a message suggesting a migration to the new API. In 2.9 the API will be removed. -#### Release a polyfill addon for this feature +#### Step #3: Release a polyfill addon for this feature A polyfill addon, similar to [ember-getowner-polyfill](https://github.com/rwjblue/ember-getowner-polyfill) will be released for this feature. This polyfill will provide the `factoryFor` @@ -234,6 +272,131 @@ This feature should be introduced along side `lookup` in the The return value of `factoryFor` should be taught as a POJO and not as an extended class. +#### Example deprecation guide: Migrating from `_lookupFactory` to `factoryFor` + +Ember owner objects have long provided an intimate API used to +fetch a factory with dependency injections. This API, `_lookupFactory`, is deprecated +in Ember 2.8 and will be removed in Ember 2.9. To ease the transition to this +new public API, a polyfill is provided with support back to Ember 2.0. + +`_lookupFactory` returned the class of resolved factory extended with +a mixin containing its injections. For example: + +```js +let factory = Ember.Object.extend(); +owner.register('my-type:a-name', factory); +let klass = owner._lookupFactory('my-type:a-name'); +klass.constructor.superclass === factory; // true +let instance = klass.create(); +``` + +`factoryFor` instead returns an object with two properties: `create` and `class`. +For example: + +```js +let factory = Ember.Object.extend(); +owner.register('my-type:a-name', factory); +let klass = owner.factoryFor('my-type:a-name'); +klass.class === factory; // true +let instance = klass.create(); +``` + +A common use-case for `_lookupFactory` was to fetch an factory with +specific needs in mind: + +* The factory needs to be created with initial values (which cannot be + provided at create-time via `lookup`. +* The instances of that factory need access to Ember's DI framework (injections, + registered dependencies). + +For example: + +```js +// app/widgets/slow.js +import Ember from 'ember'; + +export default Ember.Object.extend({ + // this instance requires access to Ember's DI framework + store: Ember.inject.service(), + + convertToModel() { + this.get('store').createRecord('widget', { + widgetType: 'slow', + name, canWobble + }); + } + +}); +``` + +```js +// app/services/widget-manager.js +import Ember from 'ember'; + +export default Ember.Service.extend({ + + init() { + this.set('widgets', []); + }, + + /* + * Create a widget of a type, and add it to the widgets array. + */ + addWidget(type, name, canWobble) { + let owner = Ember.getOwner(this); + // Use `_lookupFactory` so the `store` is accessible on instances. + let WidgetFactory = owner._lookupFactory(`widget:${type}`); + let widget = WidgetFactory.create({name, canWobble}); + this.get('widgets').pushObject(widget); + return widget; + } + +}); +``` + +For these common cases where only `create` is called on the factory, migration +to `factoryFor` is mechanical. Change `_lookupFactory` to `factoryFor` in the +above examples, and the migration would be complete. + +##### Migration of static method calls + +Factories may have had static methods or properties that were being accessed +after resolving a factory with `_lookupFactory`. For example: + +```js +// app/widgets/slow.js +import Ember from 'ember'; + +const SlowWidget = Ember.Object.extend(); +SlowWidget.reopenClass({ + SPEEDS: [ + 'slow', + 'verySlow' + ], + hasSpeed(speed) { + return this.SPEEDS.contains(speed); + } +}); + +export default SlowWidget; +``` + +```js +let factory = owner._lookupFactory('widget:slow'); +factory.SPEEDS.length; // 2 +factory.hasSpeed('slow'); // true +``` + +With `factoryFor`, access to these methods or properties should be done via +the `class` property: + +```js +let factory = owner.factoryFor('widget:slow'); +let klass = factory.class; +klass.SPEEDS.length; // 2 +klass.hasSpeed('slow'); // true +``` + # Drawbacks The main drawback to this solution is the removal of double extend. Double @@ -243,16 +406,25 @@ that some use-case relying on this behavior would get trolled in the migration to `factoryFor`, however it is unlikely. For example these cases where state is stored on the factory would no -longer be viable: +longer be scope to one instance of the owner (like one test). Instead, setting +a value on the class would set it on the registered class. + +Some real-world examples of setting state on the factory class: - ember-model - - https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L404 - - https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L457 + - https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L404 and https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L457 + with `factoryFor` will increment a shared counter across application and + container instances. - https://github.com/ebryn/ember-model/blob/master/packages/ember-model/lib/model.js#L723-L725 -- ember-data - - As far as I can tell, the big issues have been resolved! - - if attrs change between test runs (seems very unlikely) then https://github.com/emberjs/data/blob/387630db5e7daec6aac7ef8c6172358a3bd6394c/addon/-private/system/model/attr.js#L57 would be affected -- people using: + would also set properties on the base `Ember.Model` factory instead of + an extension of that class. +- ember-data + - If attrs change between test runs (seems very unlikely) then https://github.com/emberjs/data/blob/387630db5e7daec6aac7ef8c6172358a3bd6394c/addon/-private/system/model/attr.js#L57 + would be affected. The CP of `attributes` will have a value cached on the + factory, and where with `_lookupFactory`'s double-extend the cache would be + on the extended class, in `factoryFor` that CP cache will be on the + class registered as a factory. +- Any other of the following: - `lookupFactory(x).reopen` / `reopenClass` at runtime (or test time to monkey patch code) - `lookupFactory(x).something = value` From 1cb5b5908b28027eacd6066e5729b43ff79efb24 Mon Sep 17 00:00:00 2001 From: Matthew Beale Date: Fri, 22 Jul 2016 12:20:46 -0400 Subject: [PATCH 3/4] Change up the migration approach w/ factoryFor The prior approach was too fast- this edit lays out a less aggressive and more iterative approach. --- text/0000-factoryFor.md | 64 ++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/text/0000-factoryFor.md b/text/0000-factoryFor.md index 9533fe21c0..53116e2576 100644 --- a/text/0000-factoryFor.md +++ b/text/0000-factoryFor.md @@ -12,7 +12,7 @@ public API to support use cases long-served by a private API, a new API of Ember's dependency injection container has long supported fetching a factory that will be created with any injections present. Using the private API that -provided this support would allow an instance of the factory to be created +provided this support allows an instance of the factory to be created with initial values passed via `create`. For example: ```js @@ -102,16 +102,28 @@ better performance characteristics than `_lookupFactory`. # Detailed design -This feature will be added in three steps. +Throughout this document I reference Ember 2.12 as it is the next LTS at writing. This +proposal may ship for 2.12-LTS or be bumped to the next LTS. -1. Introduce `ApplicationInstance#factoryFor` in Ember 2.8 -2. Deprecate `ApplicationInstance#_lookupFactory` in Ember 2.8, remove in 2.9 -3. Release a polyfill addon for this feature +This feature will be added in these steps. -#### Step #1: Introduce `ApplicationInstance#factoryFor` in Ember 2.8 +1. In Ember introduce a `ApplicationInstance#factoryFor` based on + `_lookupFactory`. It should be documented that certain behaviors + inherent to "double extend" are not supported. In development builds + and supporting browsers, wrap return values in a Proxy. The proxy should + throw an error when any property besides `create` or `class` is accessed. +2. In the same release add a deprecation message to usage of `_lookupFactory`. + As this API is intimate it must be maintained through at least one LTS + release (2.12 at this writing). +3. In 2.13 drop `_lookupFactory` and migrate the `factoryFor` implementation to avoid + "double-extend" entirely. + +Additionally, a polyfill will be released for this feature supporting prior version of Ember. + +#### Design of `ApplicationInstance#factoryFor` A new API will be introduced. This API will return both the original base -class registed into or resolved by the container, and will also return a function +class registered into or resolved by the container, and will also return a function to generate a dependency-injected instance. For example: ```js @@ -188,15 +200,13 @@ the following: +--------------------+ +--------------------+ ``` -This means, between test runs 2 instances of `model:foo` will have a common +Between test runs 2 instances of `model:foo` will have a common shared ancestor the grandparent `Class[model/Foo]`. This implementation of `factoryFor` proposes to remove the intermediate subclass and instead have a generic factory object which holds the injections and allows for injected instances to be created. The resulting object graph would look something like this: -(between test runs the 2 instances of `model:foo` will have a shared ancestor -(parent) `Class[model/Foo]`. ``` Proposed: @@ -253,17 +263,21 @@ let factoryWithDI = owner.factoryFor('my-type:a-factory'); factoryWithDI.class === factory; ``` -#### Step #2: Deprecate `ApplicationInstance#_lookupFactory` in Ember 2.8, remove in 2.9 +##### Development-mode Proxy -In 2.8 (an LTS release) `_lookupFactory` will be deprecated with a message -suggesting a migration to the new API. In 2.9 the API will be removed. +Because many developers will simply re-write `_lookupFactory` to `factoryFor`, +it is important to provide some aid and ensure they actually complete the +migration completely (they they avoid setting state on the factory). A proxy +wrapping the factory and raising assertions when any property besides `create` +or `class` is accessed will be added in development. -#### Step #3: Release a polyfill addon for this feature +##### Releasing a polyfill A polyfill addon, similar to [ember-getowner-polyfill](https://github.com/rwjblue/ember-getowner-polyfill) will be released for this feature. This polyfill will provide the `factoryFor` -API going back to at least 2.4, provide the API and silence the deprecation -in 2.8, and be a no-op in 2.9+ where Ember provides the `factoryFor` API. +API going back to at least 2.8, provide the API and silence the deprecation +in versions before `factoryFor` is available, and be a no-op in versions where +`factoryFor` is available. # How We Teach This @@ -276,8 +290,8 @@ an extended class. Ember owner objects have long provided an intimate API used to fetch a factory with dependency injections. This API, `_lookupFactory`, is deprecated -in Ember 2.8 and will be removed in Ember 2.9. To ease the transition to this -new public API, a polyfill is provided with support back to Ember 2.0. +in Ember 2.12 and will be removed in Ember 2.13. To ease the transition to this +new public API, a polyfill is provided with support back to at least Ember 2.8. `_lookupFactory` returned the class of resolved factory extended with a mixin containing its injections. For example: @@ -430,23 +444,13 @@ Some real-world examples of setting state on the factory class: # Alternatives -No other designs have been seriously considered. +More aggressive timelines have been considered for this change. -We have considered the possibility that removing `_lookupFactory` in 2.9 +However we have considered the possibility that removing `_lookupFactory` in 2.13 (something LTS technically permits) would be too aggressive for the community of addons. Providing a polyfill is part of the strategy to handle this change. -However if the removal still proves too difficult, an alternative strategy -is possible: - -* Add `factoryFor` in 2.8 -* Deprecate `_lookupFactory` in 2.8, remove it in 3.0 -* Introduce a polyfill add for `factoryFor` valid w/ 2.4-3.0 -* In 2.9, migrate the implementation of `_lookupFactory` to be based on - `factoryFor`. There may be some slight behavior change, or performance - regression when using this API. - # Unresolved questions Are there any use-cases for the double extend not considered? From b955e33af18654284131dbe66821a309c5ca4294 Mon Sep 17 00:00:00 2001 From: Matthew Beale Date: Sun, 18 Sep 2016 13:54:16 -0700 Subject: [PATCH 4/4] Final factoryFor RFC edits --- text/0000-factoryFor.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/text/0000-factoryFor.md b/text/0000-factoryFor.md index 53116e2576..66cea6fa8d 100644 --- a/text/0000-factoryFor.md +++ b/text/0000-factoryFor.md @@ -112,13 +112,15 @@ This feature will be added in these steps. inherent to "double extend" are not supported. In development builds and supporting browsers, wrap return values in a Proxy. The proxy should throw an error when any property besides `create` or `class` is accessed. + `class` must return the registered factory, not the double extended factory. 2. In the same release add a deprecation message to usage of `_lookupFactory`. As this API is intimate it must be maintained through at least one LTS release (2.12 at this writing). 3. In 2.13 drop `_lookupFactory` and migrate the `factoryFor` implementation to avoid "double-extend" entirely. -Additionally, a polyfill will be released for this feature supporting prior version of Ember. +Additionally, a polyfill will be released for this feature supporting prior +versions of Ember. #### Design of `ApplicationInstance#factoryFor` @@ -268,8 +270,14 @@ factoryWithDI.class === factory; Because many developers will simply re-write `_lookupFactory` to `factoryFor`, it is important to provide some aid and ensure they actually complete the migration completely (they they avoid setting state on the factory). A proxy -wrapping the factory and raising assertions when any property besides `create` -or `class` is accessed will be added in development. +wrapping the return value of `factoryFor` and raising assertions when any +property besides `create` or `class` is accessed will be added in development. + +Additionally, using `instanceof` on the result of `factoryFor` should be +disallowed, causing an exception to be raised. + +A good rule of thumb is that, in development, using anything besides `class` or +`create` on the return value of `factoryFor` should fail with a helpful message. ##### Releasing a polyfill