[RFC ember-data] modelFactoryFor by runspired · Pull Request #372 · emberjs/rfcs

runspired · 2018-09-07T02:09:50Z

knownasilya · 2018-09-07T03:18:58Z

Could this work side by side with the alternative method?

runspired · 2018-09-07T03:53:46Z

runspired · 2018-09-12T22:00:21Z

We discussed this on our weekly meeting and decided to put this RFC on hold for 2 weeks until the Ember f2f in Chicago after EmberCamp. @dgeb will champion discussion on improving Ember's DI / spec'ing factory since that is outside of our domain.

Additionally, we should verify that DS.Model is no longer depended upon internally, and that a theoretical factory could simply accept modelData as a prop during initialization and that be enough.

runspired · 2018-10-04T18:10:48Z

After further discussion, we've update the proposal with the expected requirements for custom classes and use of factoryFor.

hjdivad · 2018-10-04T20:26:25Z

text/0000-ember-data-model-factory-for.md

+}
+```
+
+Users interested in providing a custom class for their `records` and whom override `modelFactoryFor`,


typo: s/whom/who

hjdivad · 2018-10-04T20:28:03Z

text/0000-ember-data-model-factory-for.md

+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)


I'd recommend not mentioning the Symbol, or rephrasing to make it clear that it's an implementation detail and not part of the contract.

eg

A recordData instance accessible via getRecordData(createArgs)

hjdivad · 2018-10-04T20:32:39Z

text/0000-ember-data-model-factory-for.md

+
+- 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`


I don't think the Ember.getOwner docs mention anything about a Symbol; let's just say the owner is accessible via Ember.getOwner

hjdivad · 2018-10-04T20:34:34Z

text/0000-ember-data-model-factory-for.md

+
+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.


Why is it necessary for the record to have a backref to the record data?

We should certainly recommend that people do this, but I don't see why we should require it. I'd also recommend we make the public api here setRecordData and not allow the record data to be set via assign, to simply the docs if nothing else.

@hjdivad implementations of Model and RecordData need to be able to convert from a record to a record-data when createRecord is called or when a relationship is mutated.

Overtime we may be able to loosen that requirement, although personally I think that even if no longer necessary it is a good one.

RE assign, since we are using a Symbol, Object.assign and the Ember polyfill "just work". Object.keys and for ... in do not work unless using Ember's Symbol polyfill.

Yes, assign can work but it will be simpler if we have a single public API, setRecordData. assign works as a side effect of using a Symbol, but this is an implementation detail (you could imagine supporting the APIs with WeakMaps for instance).

@hjdivad implementations of Model and RecordData need to be able to convert from a record to a record-data when createRecord is called or when a relationship is mutated.

That's true for DS.Models but need not be the case for other model classes. That record.set('belongsToRelationship', otherRecord) invokes recordData.setBelongsTo is an implementation detail of DS.Model -- what part of the system necessitates that this is done by all model implementations?

I think it's important that we avoid these kinds of requirements to allow users an escape valve for relationship semantics when they need it.

I think it's important that we avoid these kinds of requirements to allow users an escape valve for relationship semantics when they need it.

I feel as though we won't be able to have any-level of decent interop between various RecordData and Model implementations if we don't have this constraint. While I agree the existing relationship layer could (and should) be refactored to be less coupled, I think it's mechanics derives in part from the need for an interop story.

Yes, assign can work but it will be simpler if we have a single public API, setRecordData. assign works as a side effect of using a Symbol, but this is an implementation detail (you could imagine supporting the APIs with WeakMaps for instance).

I can get behind this idea and will update the language.

I think if we consider recordData as a "weak-mapped" thing (even if symbol based), we may be able to remove the requirement. Users would need to call recordDataFor on createArgs from within the constructor, but afterwards we could weakMap to the instance or symbolicate it ourselves and calls to recordDataFor on the instance would "just work"

I feel as though we won't be able to have any-level of decent interop between various RecordData and Model implementations if we don't have this constraint. While I agree the existing relationship layer could (and should) be refactored to be less coupled, I think it's mechanics derives in part from the need for an interop story.

That interop should be driven by RecordDatas; the relationships already deal with these objects and should be as indifferent as can be to the particular records associated with some particular RecordData.

There's no dispute that RecordDatas need to know what record to return at various times: the basic mechanism of caching an invocation of factory.create(...) works well here. Model implementations can set up edges between RecordDatas via the public RecordData API. Why is that insufficient to achieve interop?

@hjdivad because if you are a record and someone calls record.set with a different record then either the record or the record-data for the receiving record needs to be able to lookup the record-data instance associated with it. In absence of this ability, you end up needing to expose various other public interfaces to make things work. That said, I think the "weak-map after create" approach will allow us to make this work just fine.

@hjdivad because if you are a record and someone calls record.set with a different record then either the record or the record-data for the receiving record needs to be able to lookup the record-data instance associated with it.

Sure, so it's recommended to setRecordData to achieve this. But that's not the same thing as it being required. In particular I don't think the rest of the system should assume that records (besides DS.Models) have a backref of any kind to their RecordData.

hjdivad · 2018-10-17T22:00:54Z

Discussed this at the core team meeting today, and we are moving this to final comment period. We'll aim to merge this next week, barring additional comments.

runspired · 2019-02-12T19:20:23Z

As an update to this RFC, we landed an implementation only to discover several fatal flaws when interopiing with Ember's object factories that lead to a revert. A new approach is being drafted by @igorT, and this RFC should be considered dead

[RFC ember-data] modelFactoryFor

93c7703

runspired added the T-ember-data RFCs that impact the ember-data library label Sep 7, 2018

update RFC to spec out class requirements

394220f

remove no longer relevant drawback

18bdc46

hjdivad reviewed Oct 4, 2018

View reviewed changes

address feedback

89d36ae

hjdivad added the Final Comment Period label Oct 17, 2018

hjdivad merged commit 01bb599 into emberjs:master Oct 24, 2018

runspired mentioned this pull request Oct 31, 2018

Model Lifecycle Hooks #123

Closed

kategengler assigned igorT Feb 12, 2019

kategengler mentioned this pull request Feb 12, 2019

RFC #0372 - Tracking for modelFactoryFor emberjs/rfc-tracking#16

Closed

Uh oh!

Conversation

runspired commented Sep 7, 2018

Uh oh!

knownasilya commented Sep 7, 2018

Uh oh!

runspired commented Sep 7, 2018

Uh oh!

runspired commented Sep 12, 2018

Uh oh!

runspired commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjdivad commented Oct 17, 2018

Uh oh!

runspired commented Feb 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants