From 93c7703e6a62f40a4fee1d2cf67c5553c836b6f3 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 6 Sep 2018 19:07:28 -0700 Subject: [PATCH 1/4] [RFC ember-data] modelFactoryFor --- text/0000-ember-data-model-factory-for.md | 82 +++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 text/0000-ember-data-model-factory-for.md diff --git a/text/0000-ember-data-model-factory-for.md b/text/0000-ember-data-model-factory-for.md new file mode 100644 index 00000000000..ce9508bf2f9 --- /dev/null +++ b/text/0000-ember-data-model-factory-for.md @@ -0,0 +1,82 @@ +- Start Date: 2018-09-06 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# ember-data | modelFactoryFor + +## Summary + +Promote the private `store._modelFactoryFor` to public API as `store.modelFactoryFor`. + +## Motivation + +This RFC is a follow-up RFC for [#293 RecordData](/~https://github.com/emberjs/rfcs/pull/293). + +Ember differentiates between `klass` and `factory` for classes registered with the container. + At times, `ember-data` needs the `klass`, at other times, it needs the `factory`. For this reason, +`ember-data` has carried two APIs for accessing one or the other for some time. The public `modelFor` + provides access to the `klass` where schema information is stored, while the private `_modelFactoryFor` + provides access to the factory for instantiation. + +For symmetry, both of these APIs should be public. Making `modelFactoryFor` public would provide a hook + that consumers can override should they desire to provide a custom `ModelClass` as an alternative + to `DS.Model`. + +## Detailed design + +```typescript +class Store { + modelFactoryFor(modelName: String): InstantiableModelClass {} +} +``` + +Due to previous complexity in the lookup of models in `ember-data`, we previously had both `modelFactoryFor` +and `_modelFactoryFor`. Despite the naming, both of these methods were private. During a recent cleanup phase, +we unified the methods into `_modelFactoryFor` and left a deprecation in `modelFactoryFor`. This RFC proposes +un-deprecating the `modelFactoryFor` method and making it public, while deprecating the private `_modelFactoryFor`. + +More precisely: + +- `store._modelFactoryFor` becomes deprecated and calls `store.modelFactoryFor`. +- `store.modelFactoryFor` becomes un-deprecated. + +Users wishing to extend the behavior of `modelFactoryFor` could do so in the following manner: + +**services/store.js** +```js +import Store from 'ember-data/store'; + +export default Store.extend({ + modelFactoryFor(modelName) { + if (someCustomCondition) { + return someCustomFactory; + } + + return this._super(modelName); + } +}); +``` + +## How we teach this + +This API would be intended for addon-authors and power users. It is not expected +that most apps would implement custom models, much as it is not expected that most +apps would implement custom `RecordData`. The teaching story would be limited to +documenting the nature and purpose of `modelFactoryFor`. + +## Drawbacks + +- Users may try to use the hook to instantiate records on their own. Ultimately, the store + should still do the instantiating. +- We have not yet documented the APIs required to be implemented by a custom `ModelClass` and + how/where a `record` accesses the backing `RecordData` instance. + +## Alternatives + +Users could define models in `models/*.js` that utilize a custom `ModelClass`. +However, such an API for custom classes would exclude the ability to dynamically +generate classes. + +## Unresolved questions + +None From 394220fbb3e50a0542d69daebbe9ff33baf64ad1 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 4 Oct 2018 11:09:38 -0700 Subject: [PATCH 2/4] update RFC to spec out class requirements --- text/0000-ember-data-model-factory-for.md | 125 ++++++++++++++++++++-- 1 file changed, 117 insertions(+), 8 deletions(-) diff --git a/text/0000-ember-data-model-factory-for.md b/text/0000-ember-data-model-factory-for.md index ce9508bf2f9..df4ec1a249b 100644 --- a/text/0000-ember-data-model-factory-for.md +++ b/text/0000-ember-data-model-factory-for.md @@ -18,18 +18,14 @@ Ember differentiates between `klass` and `factory` for classes registered with t provides access to the `klass` where schema information is stored, while the private `_modelFactoryFor` provides access to the factory for instantiation. +We provide access to the class with `modelFor` roughly implemented as `store._modelFactoryFor(modelName).klass`. +We instantiate records from this class roughly implemented as `store._modelFactoryFor(modelName).create({ ...args })`. + For symmetry, both of these APIs should be public. Making `modelFactoryFor` public would provide a hook that consumers can override should they desire to provide a custom `ModelClass` as an alternative to `DS.Model`. ## Detailed design - -```typescript -class Store { - modelFactoryFor(modelName: String): InstantiableModelClass {} -} -``` - Due to previous complexity in the lookup of models in `ember-data`, we previously had both `modelFactoryFor` and `_modelFactoryFor`. Despite the naming, both of these methods were private. During a recent cleanup phase, we unified the methods into `_modelFactoryFor` and left a deprecation in `modelFactoryFor`. This RFC proposes @@ -40,21 +36,134 @@ More precisely: - `store._modelFactoryFor` becomes deprecated and calls `store.modelFactoryFor`. - `store.modelFactoryFor` becomes un-deprecated. +### The contract for `modelFactoryFor` + +The return value of `modelFactoryFor` MUST be the result of a call to [`applicationInstance.factoryFor`](https://www.emberjs.com/api/ember/3.4/classes/ApplicationInstance/methods/factoryFor?anchor=factoryFor) + where `applicationInstance` is the `owner` returned by using `getOwner(this)` to access the `owner` of the `store` instance. + +```typescript +interface Klass {} + +interface Factory { + klass: Klass, + create(): Klass +} + +interface FactoryMap { + [factoryName: string]: Factory +} + +declare function factoryFor(factoryName: K): FactoryMap[K]; + +interface Store { + modelFactoryFor(modelName: string): ReturnType; +} +``` + +Users interested in providing a custom class for their `records` and whom override `modelFactoryFor`, + would not need to also change `modelFor`, as this would be the `klass` accessible via the `factory`. + Users wishing to extend the behavior of `modelFactoryFor` could do so in the following manner: +**Example 1:** + **services/store.js** ```js +import { getOwner } from '@ember/application'; import Store from 'ember-data/store'; export default Store.extend({ modelFactoryFor(modelName) { if (someCustomCondition) { - return someCustomFactory; + return getOwner(this).factoryFor(someFactoryName); } return this._super(modelName); } }); +``` + + #### `Model.modelName` + + `ember-data` currently sets `modelName` onto the `klass` accessible via the `factory`. For classes that do not + inherit from `DS.Model` this would not be done, although end users may do so themselves in their implementations + if so desired. + +### What is a valid factory? + +The default export of a custom ModelClass **MUST** conform to the requirements of `Ember.factoryFor`. The requirements + of `factoryFor` are currently underspecified; however, in practice, this means that the default export is an + instantiable class with a static `create` method and an instance `destroy` method or that inherits from `EmberObject` + (which provides such methods). + +**Example 2:** + +```javascript +import { assign } from '@ember/polyfills'; + +export default class CustomModel { + constructor(createArgs) { + assign(this, createArgs); + } + destroy() { + // ... do teardown + } + static create(createArgs) { + return new this(createArgs); + } +} +``` + +**Example 3:** + +```javascript +import EmberObject from '@ember/object'; + +export default class CustomModel extends EmberObject { + constructor(createArgs) { + super(createArgs); + } +} +``` + +Custom classes for models should expect their constructor to receive a single argument: an object with *at least* + the following. + +- A `recordData` instance available on the `recordData` `Symbol` and accessible via `getRecordData` (see below) +- Any properties passed as the second arg to `createRecord` +- An `owner` available on the owner `Symbol` and accessible via `Ember.getOwner` +- Any DI injections +- any other properties that `Ember` chooses to pass to a class instantiated via `factory.create` (currently none) + +### getRecordData/setRecordData + +Every `record` (instance of the class returned by `modelFactoryFor`) will have an associated [RecordData](/~https://github.com/emberjs/rfcs/pull/293) + which contains the backing data for the id, type, attributes and relationships of that record. + +This backing data can be accessed by using the `getRecordData` util on the `record` (or on the `createArgs` passed to + a record). + +If providing a custom class that does not inherit from `EmberObject`, you *MUST* either use `assign` (see example 2) + or `setRecordData` (see example 4) to add the symbol for the backing `recordData` instance onto your record instance. + If inheriting from `EmberObject` calling `super` with the `createArgs` is sufficient. + +**Example 4** + +```javascript +import { getRecordData, setRecordData } from 'ember-data'; + +export default class CustomModel { + constructor(createArgs) { + let recordData = getRecordData(createArgs); + setRecordData(this, recordData); + } + destroy() { + // ... do teardown + } + static create(createArgs) { + return new this(createArgs); + } +} ``` ## How we teach this From 18bdc4617831f9be52651eb778971d0a57fe3fde Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 4 Oct 2018 11:23:52 -0700 Subject: [PATCH 3/4] remove no longer relevant drawback --- text/0000-ember-data-model-factory-for.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/text/0000-ember-data-model-factory-for.md b/text/0000-ember-data-model-factory-for.md index df4ec1a249b..3afc00dab6f 100644 --- a/text/0000-ember-data-model-factory-for.md +++ b/text/0000-ember-data-model-factory-for.md @@ -177,8 +177,6 @@ documenting the nature and purpose of `modelFactoryFor`. - Users may try to use the hook to instantiate records on their own. Ultimately, the store should still do the instantiating. -- We have not yet documented the APIs required to be implemented by a custom `ModelClass` and - how/where a `record` accesses the backing `RecordData` instance. ## Alternatives From 89d36aea56073a2c9cad8fe7ce414faf25fb9324 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 4 Oct 2018 14:26:43 -0700 Subject: [PATCH 4/4] address feedback --- text/0000-ember-data-model-factory-for.md | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/text/0000-ember-data-model-factory-for.md b/text/0000-ember-data-model-factory-for.md index 3afc00dab6f..7dcab899eb2 100644 --- a/text/0000-ember-data-model-factory-for.md +++ b/text/0000-ember-data-model-factory-for.md @@ -60,7 +60,7 @@ interface Store { } ``` -Users interested in providing a custom class for their `records` and whom override `modelFactoryFor`, +Users interested in providing a custom class for their `records` and who override `modelFactoryFor`, would not need to also change `modelFor`, as this would be the `klass` accessible via the `factory`. Users wishing to extend the behavior of `modelFactoryFor` could do so in the following manner: @@ -129,33 +129,34 @@ export default class CustomModel extends EmberObject { Custom classes for models should expect their constructor to receive a single argument: an object with *at least* the following. -- A `recordData` instance available on the `recordData` `Symbol` and accessible via `getRecordData` (see below) +- A `recordData` instance accessible via `getRecordData` (see below) - Any properties passed as the second arg to `createRecord` -- An `owner` available on the owner `Symbol` and accessible via `Ember.getOwner` +- An `owner` accessible via `Ember.getOwner` - Any DI injections - any other properties that `Ember` chooses to pass to a class instantiated via `factory.create` (currently none) -### getRecordData/setRecordData +### getRecordData Every `record` (instance of the class returned by `modelFactoryFor`) will have an associated [RecordData](/~https://github.com/emberjs/rfcs/pull/293) which contains the backing data for the id, type, attributes and relationships of that record. This backing data can be accessed by using the `getRecordData` util on the `record` (or on the `createArgs` passed to - a record). + a record). Using `getRecordData` on a `record` is only guaranteed after the record has been instantiated. During + instantiation, this call should be made on the `createArgs` object passed into the record. -If providing a custom class that does not inherit from `EmberObject`, you *MUST* either use `assign` (see example 2) - or `setRecordData` (see example 4) to add the symbol for the backing `recordData` instance onto your record instance. - If inheriting from `EmberObject` calling `super` with the `createArgs` is sufficient. - **Example 4** ```javascript -import { getRecordData, setRecordData } from 'ember-data'; +import { getRecordData } from 'ember-data'; export default class CustomModel { constructor(createArgs) { + // during instantiation, `recordData` is available by calling `getRecordData` on createArgs let recordData = getRecordData(createArgs); - setRecordData(this, recordData); + } + someMethod() { + // post instantiation, `recordData` is available by calling `getRecordData` on the instance + let recordData = getRecordData(this); } destroy() { // ... do teardown