Skip to content
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

Model Lifecycle Hooks #123

Closed
wants to merge 5 commits into from
Closed

Model Lifecycle Hooks #123

wants to merge 5 commits into from

Conversation

pangratz
Copy link
Member

@pangratz pangratz commented Mar 4, 2016

@pangratz pangratz added the T-ember-data RFCs that impact the ember-data library label Mar 4, 2016
import { hasMany } from "ember-data/relationships";

export default Model.extend({
comments: hasMany()o,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments: hasMany(),

@stefanpenner
Copy link
Member

Are these change hooks called synchronously, do they batch? For some of the hooks, this feels awfully like special purpose observers. Typically, we wan the world to settle, then be informed (maybe with hints) that stuff happened. So we can react and make changes against the settled state, rather then some intermediate state.

@tchak
Copy link
Member

tchak commented Mar 7, 2016

One thing that worries me is how you going to deal with model materialisation? Currently when you load but not accessing a bunch of models, only internalModels are created. If you want to run some hooks on models it means you will need to materialise all the models right away. Kinda defeats the purpose of internalModel no?

@tchak
Copy link
Member

tchak commented Mar 7, 2016

You can always look on model prototype to check if materialisation is needed, but it would mean that adding a hook will have non trivial perf implications...

@pangratz
Copy link
Member Author

pangratz commented Mar 7, 2016

Are these change hooks called synchronously, do they batch?

I would say that the hooks are invoked after the data is set on the models and all relationships have been settled. Similar to the existing didLoad, didUpdate, etc. hooks. I will update the RFC to reflect that.

For some of the hooks, this feels awfully like special purpose observers.

@stefanpenner can you clarify which hooks you mean there?


One thing that worries me is how you going to deal with model materialisation?

This is an important thing to consider and I might haven't addressed this explicitly in the RFC. Thanks for pointing that out. I would say that the hooks are only invoked on DS.Model instances, which exist at the time being. So InternalModels without a DS.Model instance won't have the hooks invoked. I.e. this means that when there is no DS.Model instance for an InternalModel, then the hooks are invoked when the model is materialized for the first time. I will update the RFC to reflect that.

@stefanpenner
Copy link
Member

@stefanpenner can you clarify which hooks you mean there?

Model Lifecycle Hooks

@stefanpenner
Copy link
Member

Are these change hooks called synchronously, do they batch?
I would say that the hooks are invoked after the data is set on the models and all relationships have been settled. Similar to the existing didLoad, didUpdate, etc. hooks. I will update the RFC to reflect that.

Ya, I think that would be good. I think this is an important (and easily overlooked) aspect of all low level API design. It is worth noting, that the side-affects a user can introduce when a hook is invoked, must be taken into account when deciding if the hook should exist, and when it should be invoked.

By exposing currently hard to observe or totally unobservable behavior, we are also locking down internal implementation to some degree. As such we shouldn't over-expose unless we must.

Please note, I am merely trying to apply a counter balance here, hopefully this thought-path will help yield best-possible solution to this problem.

Ember itself suffers from this, as several low-level API's overexpose our internals, and changing them (to fix bugs, or performance) is extremely non-trivial.

@pangratz
Copy link
Member Author

pangratz commented Mar 7, 2016

By exposing currently hard to observe or totally unobservable behavior, we are also locking down internal implementation to some degree. As such we shouldn't over-expose unless we must.

I absolutely agree. I am hoping that the discussion for this RFC clarifies which hooks are needed - maybe not all make sense.

Please note, I am merely trying to apply a counter balance here, hopefully this thought-path will help yield best-possible solution to this problem.

💯% appreciated

@fguillen
Copy link

I really would like to see an intuitive API for the "Ember Model Lifecycle". I have been struggling with this issue for a while:

I was looking for something as simple as:

  • isLoading: boolean

true if the Object is either being loaded for first time, or reloading, or being updated, I just need to know if the Object is waiting for the back to update its state.

And it has to be an observable property.

  • didLoadStart: hook

Triggered either when first load to the backend starts, when the reload starts or when the update starts.

  • didLoadFinished: hook

Triggered either when first load to the backend ends, when the reload ends or when the update ends.

@sandstrom
Copy link
Contributor

This would be great! ⛵ I've currently relied on hacks, but this looks much better.

Two thoughts:

  1. Will there be events in addition to the hooks? I find them useful with inheritance and mixins (no risk forgetting super).
    myListener: function(name, ref) { /* ... */ }.on('didReceiveRelationship')
  2. If there would be an easy way to tie these to computed properties that would be great (example below), but otherwise using the hooks to set/update some property, and then observe that instead, works too!

import Ember from 'ember';
import DS from 'ember-data';

let { attr, belongsTo, hasMany } = DS;

// CURRENT WORKAROUND
export default DS.Model.extend({
  children: hasMany('child', { async: true }),

  childrenCount: 0, // consumed by computed properties outside this class

  _childrenDidChange: function() {
    this.hasMany('children').ids().length; // will be 0
    Ember.run.next(this, '_deferred');
  }.observes('children.length'),

  _deferred() {
    this.hasMany('children').ids().length; // will be N (correct count)
    this.('childrenCount', this.hasMany('children').ids().length);
  },
});

// FUTURE
export default DS.Model.extend({
  children: hasMany('child', { async: true }),

  childrenCount: 0, // consumed by computed properties outside this class

  didReceiveRelationship: function(name, reference) {
    if (reference === 'children') {
      this.set('childrenCount', this.hasMany('children').ids().length);
    }
  },
});

How should this feature be introduced and taught to existing Ember users?
-->

This enhances the known programming model of hooks by adding one for additional
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you mean "enhances"; perhaps you mean "leverages". In any case, this is unrelated to "How to Teach This". Note also that the mere fact that some programming model is known has nothing to do with whether it's the right approach for solving some particular problem.

@pangratz pangratz changed the title ember-data: Model Lifecycle Hooks Model Lifecycle Hooks Oct 20, 2016
@runspired
Copy link
Contributor

runspired commented Oct 31, 2018

With the introduction of RecordData (RFC #293), I do not believe it is necessary anymore for us to provide lifecycle hooks to Model. Begging with the the modelFactoryFor RFC (#372 ) users may also provide custom Models which could interact with a custom RecordData and define their own lifecycles if this is truly desired.

In general, I'm fairly opposed to lifecycle hooks for objects that don't have a clear lifecycle. I think doing things at the source of the change with the full context of the change (the RecordData approach) is less likely to troll users or fall victim to order-of-operations/race conditions.

Recommend for closing.

@runspired
Copy link
Contributor

RecordData and the coming custom Record class RFCs eliminate the use cases for lifecycle hooks. Moving to FCP to close.

@runspired runspired added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Apr 24, 2019
@runspired
Copy link
Contributor

#487 is replacing #372 but otherwise the points from the comment in October stand.

@rwjblue rwjblue closed this Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed. T-ember-data RFCs that impact the ember-data library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants