New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ember Data Record Data RFC #293

Merged
merged 3 commits into from Aug 28, 2018

Conversation

Projects
None yet
@igorT
Member

igorT commented Jan 10, 2018

Rendered

@Gaurav0

This comment has been minimized.

Show comment
Hide comment
@Gaurav0

Gaurav0 Jan 10, 2018

Contributor

I feel like the name "ModelData" provides little meaning. I agree that a different name is necessary.

Contributor

Gaurav0 commented Jan 10, 2018

I feel like the name "ModelData" provides little meaning. I agree that a different name is necessary.

Show outdated Hide outdated text/0000-model-data.md Outdated
@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Jan 13, 2018

AdapterWillCommit and adapterDidCommit should be renamed to willCommit and didCommit to avoid leaking the store architecture into MD. that way a different way of getting data eg GraphQL that doesn’t use adapters can be implemented in future.

BryanCrotaz commented Jan 13, 2018

AdapterWillCommit and adapterDidCommit should be renamed to willCommit and didCommit to avoid leaking the store architecture into MD. that way a different way of getting data eg GraphQL that doesn’t use adapters can be implemented in future.

@locks locks added this to In Progress in Deprecation Candidates Jan 18, 2018

@locks locks removed this from In Progress in Deprecation Candidates Jan 18, 2018

@jakesjews

This comment has been minimized.

Show comment
Hide comment
@jakesjews

jakesjews Jan 24, 2018

A few thoughts/questions

  • Does ModelData do anything with regards to polymorphism?
  • I’m curious to see what sort of performance impact there may be. My company’s application has been running into some ember-data performance problems so I would want to make sure the impact would be very small.
  • For your three options to go about the changes I think option three is a really good idea. I really like the idea of letting the public implementation shake out after some addons are converted.
  • For the store wrapper would that require any implementation of a custom store to also implement a wrapper?

Overall I think the RFC looks really good. I'm excited to make it easier to create addons for ember-data.

jakesjews commented Jan 24, 2018

A few thoughts/questions

  • Does ModelData do anything with regards to polymorphism?
  • I’m curious to see what sort of performance impact there may be. My company’s application has been running into some ember-data performance problems so I would want to make sure the impact would be very small.
  • For your three options to go about the changes I think option three is a really good idea. I really like the idea of letting the public implementation shake out after some addons are converted.
  • For the store wrapper would that require any implementation of a custom store to also implement a wrapper?

Overall I think the RFC looks really good. I'm excited to make it easier to create addons for ember-data.

@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Jan 24, 2018

Contributor

While experimenting with an ember-data addon using model-data, we discovered a few limitations around lifecycle information. We believe three additional hooks are needed. The names I'm proposing below are likely in-need-of-improvement.

ModelData.setId()

Currently, there is no clear path for updating a new client-created record with an ID received after a save. InternalModel.setId() handles this need in ember-data today. The downside here is that the InternalModel implementation is also in charge of validation that seems critical for cache consistency: https://github.com/emberjs/data/blob/master/addon/-private/system/model/internal-model.js#L1023-L1029

It is likely that the validation logic will need to be hoisted into a store method; however, in doing so we also might be eliminating the need for setId entirely, as it would become potentially more ergonomic to pass the received ID to the model-data class via pushData.

ModelData.destroy()

Currently nothing would alert a ModelData instance that the InternalModel it is coupled to has been destroyed. Should the ModelData instance need to do additional teardown and cleanup, it would be unable to.

ModelData.canDestroy()

The naming of this hook could be greatly improved. Today, during a call to InternalModel.unloadRecord(), a check is made to see if any other InternalModel instances still require a reference to the unloaded instance. If none do, then we go ahead and fully destroy the instance. See https://github.com/emberjs/data/blob/master/addon/-private/system/model/internal-model.js#L559-L566

With ModelData, it may be that ModelData instances are entangled in a manner that InternalModel is unaware of. Moving the responsibility for determining whether it is safe to also destroy an InternalModel and ModelData after a call to unloadRecord into ModelData allows for more advanced data-storage and data-sharing arrangements between models.

cc @hjdivad @dnachev

Contributor

runspired commented Jan 24, 2018

While experimenting with an ember-data addon using model-data, we discovered a few limitations around lifecycle information. We believe three additional hooks are needed. The names I'm proposing below are likely in-need-of-improvement.

ModelData.setId()

Currently, there is no clear path for updating a new client-created record with an ID received after a save. InternalModel.setId() handles this need in ember-data today. The downside here is that the InternalModel implementation is also in charge of validation that seems critical for cache consistency: https://github.com/emberjs/data/blob/master/addon/-private/system/model/internal-model.js#L1023-L1029

It is likely that the validation logic will need to be hoisted into a store method; however, in doing so we also might be eliminating the need for setId entirely, as it would become potentially more ergonomic to pass the received ID to the model-data class via pushData.

ModelData.destroy()

Currently nothing would alert a ModelData instance that the InternalModel it is coupled to has been destroyed. Should the ModelData instance need to do additional teardown and cleanup, it would be unable to.

ModelData.canDestroy()

The naming of this hook could be greatly improved. Today, during a call to InternalModel.unloadRecord(), a check is made to see if any other InternalModel instances still require a reference to the unloaded instance. If none do, then we go ahead and fully destroy the instance. See https://github.com/emberjs/data/blob/master/addon/-private/system/model/internal-model.js#L559-L566

With ModelData, it may be that ModelData instances are entangled in a manner that InternalModel is unaware of. Moving the responsibility for determining whether it is safe to also destroy an InternalModel and ModelData after a call to unloadRecord into ModelData allows for more advanced data-storage and data-sharing arrangements between models.

cc @hjdivad @dnachev

@hjdivad

This comment has been minimized.

Show comment
Hide comment
@hjdivad

hjdivad Jan 25, 2018

Member

re #293 (comment)

ModelData.setId()

This should be handleable via adapterDidCreate as its argument is a jsonapi resource, which would include the id of the newly created resource.

ModelData.destroy()
ModelData.canDestroy()

Having thought about this a bit I strongly suspect the right approach will be to

  1. Make sure storeApisWrapper has an API for destroying records and
  2. The calls where destroy could happen are passed through modelData (eg deleteRecord, unloadRecord)

This way abstractions specific to a particular ModelData don't have to leak into Ember Data. Instead, a ModelData has an API so it can destroy records, as it's the ModelData that will have the context necessary to know when this ought to be done.

Member

hjdivad commented Jan 25, 2018

re #293 (comment)

ModelData.setId()

This should be handleable via adapterDidCreate as its argument is a jsonapi resource, which would include the id of the newly created resource.

ModelData.destroy()
ModelData.canDestroy()

Having thought about this a bit I strongly suspect the right approach will be to

  1. Make sure storeApisWrapper has an API for destroying records and
  2. The calls where destroy could happen are passed through modelData (eg deleteRecord, unloadRecord)

This way abstractions specific to a particular ModelData don't have to leak into Ember Data. Instead, a ModelData has an API so it can destroy records, as it's the ModelData that will have the context necessary to know when this ought to be done.

@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Jan 25, 2018

@hjdivad that’s only true if server doesn’t change id. Creation is two phase -

  1. createRecord
  2. Save (possibly with server changing id in response)

BryanCrotaz commented Jan 25, 2018

@hjdivad that’s only true if server doesn’t change id. Creation is two phase -

  1. createRecord
  2. Save (possibly with server changing id in response)
@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Jan 25, 2018

Please let’s remove adapter from method names to allow different comms channels in future (eg websocket push)

BryanCrotaz commented Jan 25, 2018

Please let’s remove adapter from method names to allow different comms channels in future (eg websocket push)

@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Jan 25, 2018

Contributor

@BryanCrotaz

that’s only true if server doesn’t change id. Creation is two phase

I fail to see the point you are making here, the setId conversation and the point David is making are both pointing out and accounting for the likelihood of not knowing the ID until the server responds with it.

Please let’s remove adapter from method names to allow different comms channels in future (eg websocket push)

I suspect David mistyped adapterDidCommit, and yes, we are all in agreeance that the name of that method should change to not reference adapter.

On that note, didCommit and willCommit are likely too broad, as we have both the concept of locally persist" and "remotely persist".

Contributor

runspired commented Jan 25, 2018

@BryanCrotaz

that’s only true if server doesn’t change id. Creation is two phase

I fail to see the point you are making here, the setId conversation and the point David is making are both pointing out and accounting for the likelihood of not knowing the ID until the server responds with it.

Please let’s remove adapter from method names to allow different comms channels in future (eg websocket push)

I suspect David mistyped adapterDidCommit, and yes, we are all in agreeance that the name of that method should change to not reference adapter.

On that note, didCommit and willCommit are likely too broad, as we have both the concept of locally persist" and "remotely persist".

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jan 26, 2018

Member

Interesting! A few thoughts:

  • What is the API by which an addon can provide an implementation for model data? Is it something the resolver/container finds via factoryFor, or a bespoke method?
  • Like models, can multiple model data implementations work with a single store? Are they distinguished by model type?
  • I would be ok with keeping these APIs focused on addons, for example not providing modelDataFor on the store since it means apps will start directly working with the API. You'll have more flexibility if you can constrain the users to be only addons.
  • The word "model" in Ember Data is really awkward. Sometimes it seems to refer to the schema, an abstract of any specific instance. At other times it refers to concrete instance. I think the new APIs around findRecord do a good job of shifting concrete things to be "records" instead of "models". This API should probably follow suit. Perhaps recordData.
Member

mixonic commented Jan 26, 2018

Interesting! A few thoughts:

  • What is the API by which an addon can provide an implementation for model data? Is it something the resolver/container finds via factoryFor, or a bespoke method?
  • Like models, can multiple model data implementations work with a single store? Are they distinguished by model type?
  • I would be ok with keeping these APIs focused on addons, for example not providing modelDataFor on the store since it means apps will start directly working with the API. You'll have more flexibility if you can constrain the users to be only addons.
  • The word "model" in Ember Data is really awkward. Sometimes it seems to refer to the schema, an abstract of any specific instance. At other times it refers to concrete instance. I think the new APIs around findRecord do a good job of shifting concrete things to be "records" instead of "models". This API should probably follow suit. Perhaps recordData.
@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT Jan 26, 2018

Member

@mixonic you provide a modelDataFor on the store, and decide when to _super and when not to. In most cases I would expect the modelData not to be part of the container, to avoid injections and keep it isolated. There can be multiple modelData implementations, the modelDataFor gets passed the type and id and you can decide whether to compose, call _super or use just exclusively your own.
👍 on the name

Member

igorT commented Jan 26, 2018

@mixonic you provide a modelDataFor on the store, and decide when to _super and when not to. In most cases I would expect the modelData not to be part of the container, to avoid injections and keep it isolated. There can be multiple modelData implementations, the modelDataFor gets passed the type and id and you can decide whether to compose, call _super or use just exclusively your own.
👍 on the name

@hjdivad

This comment has been minimized.

Show comment
Hide comment
@hjdivad

hjdivad Jan 29, 2018

Member

@runspired

I suspect David mistyped adapterDidCommit, and yes, we are all in agreeance that the name of that method should change to not reference adapter.

When I wrote the comment the RFC still had it named adapterDidCommit, but yes I think everyone agrees adapter should be removed from the various function names.

Member

hjdivad commented Jan 29, 2018

@runspired

I suspect David mistyped adapterDidCommit, and yes, we are all in agreeance that the name of that method should change to not reference adapter.

When I wrote the comment the RFC still had it named adapterDidCommit, but yes I think everyone agrees adapter should be removed from the various function names.

@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Jan 31, 2018

@igorT you’re suggesting the app developer is responsible for combining different addons? That doesn’t sound very easy.

Eg you might have an add on that implements local caching and another that implements websocket updates. It’s important which order the addons are _supered in. Perhaps something along the lines of a pipeline could be imagined and the addon Dev can insert the addon into the pipeline in the right place. Or a separate addon could create a common pipeline along the lines of ember-deploy

BryanCrotaz commented Jan 31, 2018

@igorT you’re suggesting the app developer is responsible for combining different addons? That doesn’t sound very easy.

Eg you might have an add on that implements local caching and another that implements websocket updates. It’s important which order the addons are _supered in. Perhaps something along the lines of a pipeline could be imagined and the addon Dev can insert the addon into the pipeline in the right place. Or a separate addon could create a common pipeline along the lines of ember-deploy

@Gaurav0

This comment has been minimized.

Show comment
Hide comment
@Gaurav0

Gaurav0 Feb 9, 2018

Contributor

I'd like to renew my objection to the name "ModelData". It provides little meaning. If nothing else, let's call it "InternalModel", that is what it is replacing and furthermore lets casual users know they probably shouldn't touch it. :)

IMO naming is important.

Contributor

Gaurav0 commented Feb 9, 2018

I'd like to renew my objection to the name "ModelData". It provides little meaning. If nothing else, let's call it "InternalModel", that is what it is replacing and furthermore lets casual users know they probably shouldn't touch it. :)

IMO naming is important.

@anlumo

This comment has been minimized.

Show comment
Hide comment
@anlumo

anlumo Feb 10, 2018

I'd like to give a perspective from someone who is using ember-data for undo/redo tracking (by recording all changes to the model in the adapter, with code to reverse them). I got it to work, but I stumbled upon two issues:

  • Right now, InternalModels leak when a record is destroyed. This leads to the issue that you can't recreate a record with the same id any more (which happens on undo, because I'm recreating the exact same record again). This is probably more an implementation issue than a spec issue, but it'd be nice if the person refactoring the ember-data internals keeps this in mind.
  • In order to restore relationships, I need to know how the relations changed in the adapter in updateRecord. Right now, you can only get the list of changes attributes, not relationships. I worked around this by tracking them myself in a separate field on the model. I think I see the possibility for this in this RFC, but I'm not 100% sure. There's a changedAttributes, but not equivalent for relationships.

anlumo commented Feb 10, 2018

I'd like to give a perspective from someone who is using ember-data for undo/redo tracking (by recording all changes to the model in the adapter, with code to reverse them). I got it to work, but I stumbled upon two issues:

  • Right now, InternalModels leak when a record is destroyed. This leads to the issue that you can't recreate a record with the same id any more (which happens on undo, because I'm recreating the exact same record again). This is probably more an implementation issue than a spec issue, but it'd be nice if the person refactoring the ember-data internals keeps this in mind.
  • In order to restore relationships, I need to know how the relations changed in the adapter in updateRecord. Right now, you can only get the list of changes attributes, not relationships. I worked around this by tracking them myself in a separate field on the model. I think I see the possibility for this in this RFC, but I'm not 100% sure. There's a changedAttributes, but not equivalent for relationships.
@danielspaniel

This comment has been minimized.

Show comment
Hide comment
@danielspaniel

danielspaniel Feb 10, 2018

I am curious if this RFC addresses an issue that comes up often in editing and creating new records.

Currently if you want to edit or create a record you have one path. You can edit the model itself and save it. You can create the model and save it. You could use a buffered proxy on edit of course, but once you start the save, all the changed attributes need to be set on the model before the the model can be saved. You can create an ember object that sort of acts like a model ( you can set properties on it ) but once you want to create you have to create record in the store and save that.

This means that saving / creating is always immediately impacting the state of the model/store and the UI. There is no way to cleanly make edits / creates that don't affect the model or store until after the model save is complete.

What I envision is a way for Ember Data users to create changes in a Model or create a new Model that looks like a model and acts like a model but is not yet in the store. They could then change this fake proxy like Model, serialize it, send the data to the backend and then update the model ( apply the changes after save success or not apply them on failure ).

So, what I am trying to say is that I would like ember data users to be able to have a
=> 'save and apply changes after success' strategy to augment the
=> 'apply changes immediately and save' strategy ( which is the current and only one )

Would this RFC help in that regard? Or would there be other work to be done?

danielspaniel commented Feb 10, 2018

I am curious if this RFC addresses an issue that comes up often in editing and creating new records.

Currently if you want to edit or create a record you have one path. You can edit the model itself and save it. You can create the model and save it. You could use a buffered proxy on edit of course, but once you start the save, all the changed attributes need to be set on the model before the the model can be saved. You can create an ember object that sort of acts like a model ( you can set properties on it ) but once you want to create you have to create record in the store and save that.

This means that saving / creating is always immediately impacting the state of the model/store and the UI. There is no way to cleanly make edits / creates that don't affect the model or store until after the model save is complete.

What I envision is a way for Ember Data users to create changes in a Model or create a new Model that looks like a model and acts like a model but is not yet in the store. They could then change this fake proxy like Model, serialize it, send the data to the backend and then update the model ( apply the changes after save success or not apply them on failure ).

So, what I am trying to say is that I would like ember data users to be able to have a
=> 'save and apply changes after success' strategy to augment the
=> 'apply changes immediately and save' strategy ( which is the current and only one )

Would this RFC help in that regard? Or would there be other work to be done?

@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Feb 10, 2018

Contributor

@danielspaniel it does not directly but it does open the door for adding to do so

Contributor

runspired commented Feb 10, 2018

@danielspaniel it does not directly but it does open the door for adding to do so

@anlumo

This comment has been minimized.

Show comment
Hide comment
@anlumo

anlumo Feb 10, 2018

@danielspaniel What you'd actually want for this is operational transforms instead of straight record objects, because you want to track changes and apply them to the model later. Incidentally, this is the same as I need for undo/redo support.

I'm afraid that this is something that doesn't really fit into ember's concepts, so this might not work for ember-data.

anlumo commented Feb 10, 2018

@danielspaniel What you'd actually want for this is operational transforms instead of straight record objects, because you want to track changes and apply them to the model later. Incidentally, this is the same as I need for undo/redo support.

I'm afraid that this is something that doesn't really fit into ember's concepts, so this might not work for ember-data.

@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Feb 10, 2018

Contributor

I'm afraid that this is something that doesn't really fit into ember's concepts, so this might not work for ember-data.

Heartily disagree. Ember is just Javascript and Orbit handles this concept quite nicely. This RFC will enable orbit+Ember-Data to become a potential reality. But yes, in Ember Data today OT is not something we do or make easy for others to do.

We've been discussing for some time what the right boundary between adapter and store would be in the future, a future this RFC helps us to explore. Major themes have included removing some of the restrictions on state keeping and request management that the store unnaturally imposes today. I'm optimistic that this is a huge step forward to enabling Ember Data to evolve as a project as needed.

Contributor

runspired commented Feb 10, 2018

I'm afraid that this is something that doesn't really fit into ember's concepts, so this might not work for ember-data.

Heartily disagree. Ember is just Javascript and Orbit handles this concept quite nicely. This RFC will enable orbit+Ember-Data to become a potential reality. But yes, in Ember Data today OT is not something we do or make easy for others to do.

We've been discussing for some time what the right boundary between adapter and store would be in the future, a future this RFC helps us to explore. Major themes have included removing some of the restrictions on state keeping and request management that the store unnaturally imposes today. I'm optimistic that this is a huge step forward to enabling Ember Data to evolve as a project as needed.

@mmpestorich

This comment has been minimized.

Show comment
Hide comment
@mmpestorich

mmpestorich Feb 13, 2018

I really have fundamental problems with how ED has evolved over recent years. The multiple refactors that ED has gone through, IMHO, have made it less extensible and maintainable. I am generally in favor of this RFC but have some concerns I'd like to put out there.

In its current state, ED "assumes" too much:

  1. Payload/Deserialized Format. The Store currently expects incoming data (via store.push) to follow the JSON API spec. The JSON API spec is a wire protocol. It should not be consumed directly by the Store as currently it is. This effectively requires multiple deserialization steps: payload to JSON API payload to InternalModel which then is further wrapped by Model.
  2. Transport mechanisms. The JSONAPI spec is geared toward HTTP/REST (i.e. links: /people), which doesn't necessarily play nice with custom protocols over other transports (i.e. NoSQL-based or other custom wire protocols over say WebSockets). This makes it difficult to deserialize some protocols into JSON API for the Store to consume. Internals shouldn't rely on a spec that is bias toward a particular convention.
  3. Control over dirtiness (including relationships), which is missing from current ED, should be part of ModelData (similar to what I've added in emberjs/data#4361).
  4. Control over rollback (including relationships), which is missing from current ED, should also be part of ModelData (also similar to what I've added in emberjs/data#4361).

I'd really, really, really like to see all internal use of the JSON API spec eleminated and 'just' deserialize an incoming payload directly into ModelData which the Store would then use internally. I was a little unclear when reading the RFC if this was the intent of ModelData or not (though it appears that the intent is to still "back" ModelData by a JSON API payload, which I really don't like for the reasons discussed herein).

From a highlevel, I believe:

The payload, should consist of incoming or outgoing data in any user-specified format (i.e. JSON API or something/anything else).

The Serializer should be able to deserialize an arbitraily formatted payload directly to ModelData. And conversly, serialize ModelData back to a payload of arbitary format.

The ModelData should be responsible for accessing attributes and relationships as well as maintaining dirtiness.

The Store should consume (via store.push) ModelData, not a JSON API formatted payload.

The Adapter should only be concerned with sending and receiving data from an arbitrary backend via an arbitrary transport.

Ideally, I'd like to be able to implement three interfaces to support my particular backend needs:

  • Adapter (i.e. WebSocketAdapter)
  • Serializer (i.e. MyCustomSerializer that transforms a payload of arbitray format to ModelData)
  • ModelData (i.e. MyCustomModelData that handles dirtiness and resolving relationships in a way that confroms to my backend)

mmpestorich commented Feb 13, 2018

I really have fundamental problems with how ED has evolved over recent years. The multiple refactors that ED has gone through, IMHO, have made it less extensible and maintainable. I am generally in favor of this RFC but have some concerns I'd like to put out there.

In its current state, ED "assumes" too much:

  1. Payload/Deserialized Format. The Store currently expects incoming data (via store.push) to follow the JSON API spec. The JSON API spec is a wire protocol. It should not be consumed directly by the Store as currently it is. This effectively requires multiple deserialization steps: payload to JSON API payload to InternalModel which then is further wrapped by Model.
  2. Transport mechanisms. The JSONAPI spec is geared toward HTTP/REST (i.e. links: /people), which doesn't necessarily play nice with custom protocols over other transports (i.e. NoSQL-based or other custom wire protocols over say WebSockets). This makes it difficult to deserialize some protocols into JSON API for the Store to consume. Internals shouldn't rely on a spec that is bias toward a particular convention.
  3. Control over dirtiness (including relationships), which is missing from current ED, should be part of ModelData (similar to what I've added in emberjs/data#4361).
  4. Control over rollback (including relationships), which is missing from current ED, should also be part of ModelData (also similar to what I've added in emberjs/data#4361).

I'd really, really, really like to see all internal use of the JSON API spec eleminated and 'just' deserialize an incoming payload directly into ModelData which the Store would then use internally. I was a little unclear when reading the RFC if this was the intent of ModelData or not (though it appears that the intent is to still "back" ModelData by a JSON API payload, which I really don't like for the reasons discussed herein).

From a highlevel, I believe:

The payload, should consist of incoming or outgoing data in any user-specified format (i.e. JSON API or something/anything else).

The Serializer should be able to deserialize an arbitraily formatted payload directly to ModelData. And conversly, serialize ModelData back to a payload of arbitary format.

The ModelData should be responsible for accessing attributes and relationships as well as maintaining dirtiness.

The Store should consume (via store.push) ModelData, not a JSON API formatted payload.

The Adapter should only be concerned with sending and receiving data from an arbitrary backend via an arbitrary transport.

Ideally, I'd like to be able to implement three interfaces to support my particular backend needs:

  • Adapter (i.e. WebSocketAdapter)
  • Serializer (i.e. MyCustomSerializer that transforms a payload of arbitray format to ModelData)
  • ModelData (i.e. MyCustomModelData that handles dirtiness and resolving relationships in a way that confroms to my backend)
@christophersansone

This comment has been minimized.

Show comment
Hide comment
@christophersansone

christophersansone Feb 13, 2018

Thanks for all the great work here, everyone. Really excited to see some major ED improvements on the way. And I realize how difficult it is to evolve without heavily breaking backwards compatibility, so 👍 on all of your work. I would like to share my biggest pain point with ED and see if this will have potential for resolution.

As Ember has evolved over the years, its philosophy has shifted from two-way bindings everywhere to a more unidirectional data flow ("data down actions up"). This made Ember application code much easier to reason about and reduced the number of unintended side-effects from changes to bound properties. Ember Data, however, still has a two-way binding feel that causes some fairly severe long-lived state problems. Some examples:

  • A user navigates to posts/index and sees the list of 14 posts. The user then navigates to posts/new to create a new post. The route's model() returns store.createRecord('post'). The user then decides not to finish and navigates back to posts/index. The user now sees 15 posts, because the incomplete, unsaved model is still in the store. Obviously there are ways for the developer to address this, e.g. destroying the record upon deactivate, creating or filtering the list for those that contain an ID, but the reality is that the default behavior feels incorrect. As a developer, I want to be able to create an instance of a model that is not contained in the store until it has been successfully saved, so that unsaved models will be discarded when they become out of scope.

  • A user navigates to posts/10 and sees the content of the post. The user then decides to make a change and goes to posts/10/edit. The user starts to make changes to the content and then changes his mind and navigates back to posts/10. The default behavior is that the unsaved changes appear on this page, which again seems very incorrect. As a developer, I want to be able to make local changes to a model that are only persisted to the store upon successfully saving it.

Out of the box, this can be very error prone. It feels like the two-way binding problems because a model can change by either the adapter or locally, resulting in many side-effects. There are several Ember addons that try to address this by buffering the changes in a separate object, but they all have some limitations and problems, such as relationships not being rolled back properly, computed properties not using the adjusted data, etc. They do their best to work around this problem, because it cannot otherwise be solved without some fundamental changes to Ember Data.

When you examine a server-side database + ORM, this problem does not exist. Consider this Ruby on Rails pseudo example:

def update
  post = Post.find(params[:id])
  post.title = params[:title]
  if post.save
    render post
  else
    render post.errors, status: 422
  end
end

Here, ActiveRecord requests the record from the database. The database will always only contain known, valid records. The post variable is a copy of the database record, and setting title only changes the instance of that copy until the record is committed. If the change is valid, then it is saved, and post now contains the updated record from the database. If not, the request is rejected, the data is discarded, and the database remains pristine.

That said, my preferred data flow would be something like this:

  • The store's data is unidirectional: it only comes from the server (or more generically, the "adapter"). The application can fetch data from the server at any time, and data from the server can be pushed into the store at any time. As a result, the models are read-only. This way, the developer has absolute confidence that any data in the store is actual, valid, pristine data from the server.

  • When a record is to be edited, the route / controller asks the store to provide some sort of editable copy of the model. Changes to the copy are made without affecting the underlying model in the store. Upon saving, the updated version is sent to the server, still without touching the underlying model. When the request is successful, the store consumes the updated data, and the underlying model is updated. If the request fails, the changes are still isolated to the copy of the model, while the store still contains the valid version.

I believe the more we can adopt this strategy, the more natural Ember Data will feel and the less error prone it will be.

I understand that this RFC does not address all of this... but do the proposed changes at least make this feasible, and if so, can you briefly explain how?

christophersansone commented Feb 13, 2018

Thanks for all the great work here, everyone. Really excited to see some major ED improvements on the way. And I realize how difficult it is to evolve without heavily breaking backwards compatibility, so 👍 on all of your work. I would like to share my biggest pain point with ED and see if this will have potential for resolution.

As Ember has evolved over the years, its philosophy has shifted from two-way bindings everywhere to a more unidirectional data flow ("data down actions up"). This made Ember application code much easier to reason about and reduced the number of unintended side-effects from changes to bound properties. Ember Data, however, still has a two-way binding feel that causes some fairly severe long-lived state problems. Some examples:

  • A user navigates to posts/index and sees the list of 14 posts. The user then navigates to posts/new to create a new post. The route's model() returns store.createRecord('post'). The user then decides not to finish and navigates back to posts/index. The user now sees 15 posts, because the incomplete, unsaved model is still in the store. Obviously there are ways for the developer to address this, e.g. destroying the record upon deactivate, creating or filtering the list for those that contain an ID, but the reality is that the default behavior feels incorrect. As a developer, I want to be able to create an instance of a model that is not contained in the store until it has been successfully saved, so that unsaved models will be discarded when they become out of scope.

  • A user navigates to posts/10 and sees the content of the post. The user then decides to make a change and goes to posts/10/edit. The user starts to make changes to the content and then changes his mind and navigates back to posts/10. The default behavior is that the unsaved changes appear on this page, which again seems very incorrect. As a developer, I want to be able to make local changes to a model that are only persisted to the store upon successfully saving it.

Out of the box, this can be very error prone. It feels like the two-way binding problems because a model can change by either the adapter or locally, resulting in many side-effects. There are several Ember addons that try to address this by buffering the changes in a separate object, but they all have some limitations and problems, such as relationships not being rolled back properly, computed properties not using the adjusted data, etc. They do their best to work around this problem, because it cannot otherwise be solved without some fundamental changes to Ember Data.

When you examine a server-side database + ORM, this problem does not exist. Consider this Ruby on Rails pseudo example:

def update
  post = Post.find(params[:id])
  post.title = params[:title]
  if post.save
    render post
  else
    render post.errors, status: 422
  end
end

Here, ActiveRecord requests the record from the database. The database will always only contain known, valid records. The post variable is a copy of the database record, and setting title only changes the instance of that copy until the record is committed. If the change is valid, then it is saved, and post now contains the updated record from the database. If not, the request is rejected, the data is discarded, and the database remains pristine.

That said, my preferred data flow would be something like this:

  • The store's data is unidirectional: it only comes from the server (or more generically, the "adapter"). The application can fetch data from the server at any time, and data from the server can be pushed into the store at any time. As a result, the models are read-only. This way, the developer has absolute confidence that any data in the store is actual, valid, pristine data from the server.

  • When a record is to be edited, the route / controller asks the store to provide some sort of editable copy of the model. Changes to the copy are made without affecting the underlying model in the store. Upon saving, the updated version is sent to the server, still without touching the underlying model. When the request is successful, the store consumes the updated data, and the underlying model is updated. If the request fails, the changes are still isolated to the copy of the model, while the store still contains the valid version.

I believe the more we can adopt this strategy, the more natural Ember Data will feel and the less error prone it will be.

I understand that this RFC does not address all of this... but do the proposed changes at least make this feasible, and if so, can you briefly explain how?

@danielspaniel

This comment has been minimized.

Show comment
Hide comment
@danielspaniel

danielspaniel Feb 13, 2018

@christophersansone .. I don't mean to sound like you mom, but I asked the same question already, and it was already answered. If you read the post about 3 days ago that has this in it:

So, what I am trying to say is that I would like ember data users to be able to have a
=> 'save and apply changes after success' strategy to augment the
=> 'apply changes immediately and save' strategy ( which is the current and only one )

danielspaniel commented Feb 13, 2018

@christophersansone .. I don't mean to sound like you mom, but I asked the same question already, and it was already answered. If you read the post about 3 days ago that has this in it:

So, what I am trying to say is that I would like ember data users to be able to have a
=> 'save and apply changes after success' strategy to augment the
=> 'apply changes immediately and save' strategy ( which is the current and only one )

@christophersansone

This comment has been minimized.

Show comment
Hide comment
@christophersansone

christophersansone Feb 13, 2018

@danielspaniel Thanks Mom! 😉 Okay yeah sorry, some different semantics but generally the same question. It seems to me to mostly boil down to being able to create an actual instance of a model that is detached from the store. Setting its data to copy an existing record seems trivial at that point. Then we can have our own patterns for sending data through the adapter and pushing the changes back to the store. Could even be an addon as the pattern becomes refined. But it's an impossibility now due to model instances being so tightly coupled to the store.

christophersansone commented Feb 13, 2018

@danielspaniel Thanks Mom! 😉 Okay yeah sorry, some different semantics but generally the same question. It seems to me to mostly boil down to being able to create an actual instance of a model that is detached from the store. Setting its data to copy an existing record seems trivial at that point. Then we can have our own patterns for sending data through the adapter and pushing the changes back to the store. Could even be an addon as the pattern becomes refined. But it's an impossibility now due to model instances being so tightly coupled to the store.

@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Feb 13, 2018

@christophersansone @danielspaniel
I used a very nice Delphi ORM (Bold) back in the day which had the concept of transactions in the object space. So in your example:

  1. Create transaction
  2. Create new object (and child objects, relations etc)
  3. Save: Commit transaction
  4. Cancel: Rollback transaction

Then we can look at whether changes in uncommitted transactions are visible to the rest of the system - this would be a nice config on the store so you can have the best of both worlds.

Also means you can Save a transaction back to the server, while a second transaction is in progress, so it's easy to command sparse PATCHes.

BryanCrotaz commented Feb 13, 2018

@christophersansone @danielspaniel
I used a very nice Delphi ORM (Bold) back in the day which had the concept of transactions in the object space. So in your example:

  1. Create transaction
  2. Create new object (and child objects, relations etc)
  3. Save: Commit transaction
  4. Cancel: Rollback transaction

Then we can look at whether changes in uncommitted transactions are visible to the rest of the system - this would be a nice config on the store so you can have the best of both worlds.

Also means you can Save a transaction back to the server, while a second transaction is in progress, so it's easy to command sparse PATCHes.

@Gaurav0

This comment has been minimized.

Show comment
Hide comment
@Gaurav0

Gaurav0 Feb 13, 2018

Contributor

Hi all, I agree that Ember Data needs something like a transactional model. However, addons can and do address this. See ember-buffered-proxy, ember-changeset, and others. I think all of this is out of scope for this RFC. When the addon community comes up with a solution to this problem that the community commonly uses and agrees is best and actually workable, I think we can RFC that. I think we should continue to discuss this elsewhere, perhaps open a separate issue on this repo.

Contributor

Gaurav0 commented Feb 13, 2018

Hi all, I agree that Ember Data needs something like a transactional model. However, addons can and do address this. See ember-buffered-proxy, ember-changeset, and others. I think all of this is out of scope for this RFC. When the addon community comes up with a solution to this problem that the community commonly uses and agrees is best and actually workable, I think we can RFC that. I think we should continue to discuss this elsewhere, perhaps open a separate issue on this repo.

@danielspaniel

This comment has been minimized.

Show comment
Hide comment
@danielspaniel

danielspaniel Feb 13, 2018

@christophersansone , @BryanCrotaz .. we are 100% on the same wavelength, and when the time comes and ember data is updated with this abstraction, I am going to jump on this for an addon, and I'd be happy if you would join me. Just bring some apple juice , and a few slices of lime and we can get on to making this a reality.

danielspaniel commented Feb 13, 2018

@christophersansone , @BryanCrotaz .. we are 100% on the same wavelength, and when the time comes and ember data is updated with this abstraction, I am going to jump on this for an addon, and I'd be happy if you would join me. Just bring some apple juice , and a few slices of lime and we can get on to making this a reality.

@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Feb 13, 2018

@Gaurav0
Sort of supported by addons - changeset and buffered-proxy only deal with a single object, not arbitrary changes to the object space. If ModelData has native support for transactions (e.g. passing optional transaction id in set() - completely non trivial) then this would be the best thing. Ideally a simple change is implemented as a single step transaction making transactions the core.

BryanCrotaz commented Feb 13, 2018

@Gaurav0
Sort of supported by addons - changeset and buffered-proxy only deal with a single object, not arbitrary changes to the object space. If ModelData has native support for transactions (e.g. passing optional transaction id in set() - completely non trivial) then this would be the best thing. Ideally a simple change is implemented as a single step transaction making transactions the core.

@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Feb 13, 2018

In order to achieve all these lovely ideas, ModelData needs to have a pretty maximal footprint within the Store.

BryanCrotaz commented Feb 13, 2018

In order to achieve all these lovely ideas, ModelData needs to have a pretty maximal footprint within the Store.

@christophersansone

This comment has been minimized.

Show comment
Hide comment
@christophersansone

christophersansone Feb 13, 2018

@BryanCrotaz 👍 for Delphi back in the day! Yes optimally a larger long-term solution would cover more than a single record. My current opinion is that it can be important but less critical, particularly because JSON:API does not have native support for batch updates (yet).

@Gaurav0 Wholeheardly disagree, as stated in my initial post. These current addons are workarounds that address some but not all of the problems, and they introduce some of their own problems: relationships may not work properly, computed properties may not behave as expected, etc. A new Ember developer will notice things don't work as expected, run into problems, attempt to resolve on their own, search for addons, and run into different problems. I believe this issue is at very core of ED. Much like when the Ember core team changed direction from two-way bindings to Data Down Actions Up... they didn't just say "hey, if you think this is a problem, go find an addon to fix it". They saw a fundamental problem with Ember, they discovered a better approach, and they addressed it at the core. I don't necessarily expect the ED team to necessarily rewire everything, but at least hopefully offer the proper abstractions so that developers and addon authors can do so.

We've all been there, right? "Hmm, what is this empty record doing in my list? Oh, I created it but didn't save it, and it's showing up in the list? How weird. I need to find a way to fix that."

christophersansone commented Feb 13, 2018

@BryanCrotaz 👍 for Delphi back in the day! Yes optimally a larger long-term solution would cover more than a single record. My current opinion is that it can be important but less critical, particularly because JSON:API does not have native support for batch updates (yet).

@Gaurav0 Wholeheardly disagree, as stated in my initial post. These current addons are workarounds that address some but not all of the problems, and they introduce some of their own problems: relationships may not work properly, computed properties may not behave as expected, etc. A new Ember developer will notice things don't work as expected, run into problems, attempt to resolve on their own, search for addons, and run into different problems. I believe this issue is at very core of ED. Much like when the Ember core team changed direction from two-way bindings to Data Down Actions Up... they didn't just say "hey, if you think this is a problem, go find an addon to fix it". They saw a fundamental problem with Ember, they discovered a better approach, and they addressed it at the core. I don't necessarily expect the ED team to necessarily rewire everything, but at least hopefully offer the proper abstractions so that developers and addon authors can do so.

We've all been there, right? "Hmm, what is this empty record doing in my list? Oh, I created it but didn't save it, and it's showing up in the list? How weird. I need to find a way to fix that."

@Gaurav0

This comment has been minimized.

Show comment
Hide comment
@Gaurav0

Gaurav0 Feb 13, 2018

Contributor

@christophersansone Believe me, I know current addons aren't enough. Recently I wrote a promise proxy version of ember-buffered-array-proxy for async hasMany array-like objects. I am trying to get permission to open source it.

But these one off solutions aren't enough for dealing with all relationships. What I'm saying is that these problems can't be solved just by the Model Data RFC and should be discussed in a separate issue, so as to allow people to discuss and comment on this RFC's issues here without searching through a discussion of a larger problem.

Contributor

Gaurav0 commented Feb 13, 2018

@christophersansone Believe me, I know current addons aren't enough. Recently I wrote a promise proxy version of ember-buffered-array-proxy for async hasMany array-like objects. I am trying to get permission to open source it.

But these one off solutions aren't enough for dealing with all relationships. What I'm saying is that these problems can't be solved just by the Model Data RFC and should be discussed in a separate issue, so as to allow people to discuss and comment on this RFC's issues here without searching through a discussion of a larger problem.

@christophersansone

This comment has been minimized.

Show comment
Hide comment
@christophersansone

christophersansone Feb 13, 2018

@Gaurav0 Thanks for the clarification. Agreed that this discussion is starting to take a life of its own.

...these problems can't be solved just by the Model Data RFC...

I respectfully disagree. My understanding of this RFC is to refactor the internal models and expose some public APIs so that the model classes are less brittle and can be more adaptable. If this refactoring provides the ability for a model instance to exist outside of the store, then this is the first step. If not, I would ask that they consider some ability to solve for this. This seems to be very much within the context of this RFC. I wrote a lot more about the problem and my ideal solution, in an effort to fully explain the need.

christophersansone commented Feb 13, 2018

@Gaurav0 Thanks for the clarification. Agreed that this discussion is starting to take a life of its own.

...these problems can't be solved just by the Model Data RFC...

I respectfully disagree. My understanding of this RFC is to refactor the internal models and expose some public APIs so that the model classes are less brittle and can be more adaptable. If this refactoring provides the ability for a model instance to exist outside of the store, then this is the first step. If not, I would ask that they consider some ability to solve for this. This seems to be very much within the context of this RFC. I wrote a lot more about the problem and my ideal solution, in an effort to fully explain the need.

@BryanCrotaz

This comment has been minimized.

Show comment
Hide comment
@BryanCrotaz

BryanCrotaz Feb 14, 2018

Further to @christophersansone maybe we need a list of the types of addon that we're looking to support with this new public API.

I'll make a start:

  • ad hoc data push (e.g. websockets)
  • batch updates
  • briefcase model (e.g. cache data in IndexedDB to work offline)
  • object space transaction support
  • save strategy - do new objects appear in the store or not

We then need to make sure that the properties and actions these addons would need to intercept / override are present in the public API

BryanCrotaz commented Feb 14, 2018

Further to @christophersansone maybe we need a list of the types of addon that we're looking to support with this new public API.

I'll make a start:

  • ad hoc data push (e.g. websockets)
  • batch updates
  • briefcase model (e.g. cache data in IndexedDB to work offline)
  • object space transaction support
  • save strategy - do new objects appear in the store or not

We then need to make sure that the properties and actions these addons would need to intercept / override are present in the public API

@joelalejandro

This comment has been minimized.

Show comment
Hide comment
@joelalejandro

joelalejandro Feb 15, 2018

Hello, mundo 👋 Finally got around to have a look at this RFC. I'm happy that work is being done to try to bring order to ED's dirty laundry :P

I have a couple of comments, hopefully they weren't addressed by/in earlier comments:

  • Seems to me that a good name for ModelData would be BackingModel (since it's mentioned in this regard in so many places in the RFC).

  • The RFC mentions that in order to get a ModelData instance from the DS.Model.get() method, we would need an _internalMethodForGettingTheCorrespondingModelData. Since DS.Model has a reference to store in its scope (or at least it appears that way), shouldn't we directly use store.modelDataFor() instead of defining another method for DS.Model?

  • In regards to Versioning and stability, I think option 2 (Move from the internals to public ModelData in a single release cycle) is more desirable. Sounds to me that changes would get significant feedback from its users more quickly in that way. Granted, it's riskier, but I believe it'd enable faster iterations to keep on evolving the refactor.

  • Concerning _ED in meta, while it is semantically valid to have a property like that there, since meta can be used to include non-standard meta-information, we're altering the original payload (I imagine a dev would wonder "now where did that come from?"). I believe it's best to save _ED outside of meta, since it's not necessarily related to the server-side payload.

joelalejandro commented Feb 15, 2018

Hello, mundo 👋 Finally got around to have a look at this RFC. I'm happy that work is being done to try to bring order to ED's dirty laundry :P

I have a couple of comments, hopefully they weren't addressed by/in earlier comments:

  • Seems to me that a good name for ModelData would be BackingModel (since it's mentioned in this regard in so many places in the RFC).

  • The RFC mentions that in order to get a ModelData instance from the DS.Model.get() method, we would need an _internalMethodForGettingTheCorrespondingModelData. Since DS.Model has a reference to store in its scope (or at least it appears that way), shouldn't we directly use store.modelDataFor() instead of defining another method for DS.Model?

  • In regards to Versioning and stability, I think option 2 (Move from the internals to public ModelData in a single release cycle) is more desirable. Sounds to me that changes would get significant feedback from its users more quickly in that way. Granted, it's riskier, but I believe it'd enable faster iterations to keep on evolving the refactor.

  • Concerning _ED in meta, while it is semantically valid to have a property like that there, since meta can be used to include non-standard meta-information, we're altering the original payload (I imagine a dev would wonder "now where did that come from?"). I believe it's best to save _ED outside of meta, since it's not necessarily related to the server-side payload.

@christophersansone

This comment has been minimized.

Show comment
Hide comment
@christophersansone

christophersansone Feb 19, 2018

ICYMI, @dgeb answered a lot of these recent comments here. Definitely worth the read, but TLDR this RFC is the first step towards unlocking some of these different patterns.

christophersansone commented Feb 19, 2018

ICYMI, @dgeb answered a lot of these recent comments here. Definitely worth the read, but TLDR this RFC is the first step towards unlocking some of these different patterns.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Mar 9, 2018

Member

@emberjs/ember-data-contributors - Where are we at with this? Anything needed to do before landing? Any outstanding concerns?

Member

rwjblue commented Mar 9, 2018

@emberjs/ember-data-contributors - Where are we at with this? Anything needed to do before landing? Any outstanding concerns?

@dgeb dgeb changed the title from Ember Data Model Data RFC to Ember Data Record Data RFC Mar 14, 2018

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Jun 21, 2018

Member

In order to merge this, we still need to update the text to reflect the name change from ModelData to RecordData. @igorT plans to make this change in the coming days.

Member

dgeb commented Jun 21, 2018

In order to merge this, we still need to update the text to reflect the name change from ModelData to RecordData. @igorT plans to make this change in the coming days.

@knownasilya

This comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Aug 8, 2018

Contributor

Ping @igorT 😄

Contributor

knownasilya commented Aug 8, 2018

Ping @igorT 😄

@runspired runspired merged commit 36e87bf into emberjs:master Aug 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@knownasilya

This comment has been minimized.

Show comment
Hide comment
@knownasilya
Contributor

knownasilya commented Aug 28, 2018

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment