Setup data without triggering materialization. #827

Merged
merged 2 commits into from May 10, 2013

Conversation

Projects
None yet
7 participants
@mmpestorich

A model's data is lazily initialized but in doing so currently triggers the materialization process. This is a problem if something tries to access the model's data while it is in a loading state - for example, binding to a property on an record that is not yet loaded currently has this affect:

var record = SomeModel.find(1);

Will put the record into a loading state:

record.stateManager.currentState.path === 'rootState.loading' //true

In the meantime, before data has been returned from the server:

var MyObject = Ember.Object.extend({
    init: function() {
        set(this, 'valueBinding', Ember.bind(this, 'value', 'record.' + get(this, 'property')));
    }
});
MyObject.create({ record: record, property: 'name'})

The binding will eventually attempt to get the model's data, the model's data is lazily initialized (eventually in the store's materializeData) and a materializingData event is sent to the model causing the model's state to transition:

record.stateManager.currentState.path === 'rootState.loaded.materializing.firstTime' //true

...and then a finishedMaterializing event is sent causing a transition to:

record.stateManager.currentState.path === 'rootState.loaded.saved' //true

The problem is that the model's data hasn't actually come back from the server yet and as such should not be in a loaded state in which isLoaded equals true and didLoad has been triggered resolving the load promise.

I believe that materialization should only be triggered by transitioning out of the loading state and not by simply accessing a model's data. This commit allows data to be lazily init'd without triggering materialization.

@thomasboyt

This comment has been minimized.

Show comment
Hide comment
@thomasboyt

thomasboyt Apr 3, 2013

Contributor

👍 Lost an entire morning to this.

Contributor

thomasboyt commented Apr 3, 2013

👍 Lost an entire morning to this.

@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Apr 3, 2013

Member

Sounds good; happy to merge if we can get a unit test. :)

Member

tomdale commented Apr 3, 2013

Sounds good; happy to merge if we can get a unit test. :)

@kristianmandrup

This comment has been minimized.

Show comment
Hide comment

👍

@ghost ghost assigned igorT Apr 14, 2013

@mmpestorich

This comment has been minimized.

Show comment
Hide comment
@mmpestorich

mmpestorich Apr 15, 2013

I'll get a test up this week.

I'll get a test up this week.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak May 5, 2013

Member

@mmpestorich would you have a time soon to add a test for this patch? This is a critical regression, we need to get it in as soon as possible. If you do not have time I am happy to provide a test.

Member

tchak commented May 5, 2013

@mmpestorich would you have a time soon to add a test for this patch? This is a critical regression, we need to get it in as soon as possible. If you do not have time I am happy to provide a test.

@mmpestorich

This comment has been minimized.

Show comment
Hide comment
@mmpestorich

mmpestorich May 5, 2013

Sorry. I forgot about this. I should have the time to push up a test on this later tonight.

Sorry. I forgot about this. I should have the time to push up a test on this later tonight.

@mmpestorich

This comment has been minimized.

Show comment
Hide comment
@mmpestorich

mmpestorich May 6, 2013

I believe that should do it

I believe that should do it

@inkredabull

This comment has been minimized.

Show comment
Hide comment
@inkredabull

inkredabull May 10, 2013

So glad to see this being addressed. We had an acceptance test failing and I took me a while to get to the bottom of why.

In the browser (manually) and under Selenium, functionality was fine. The spec failed under Poltergeist; kept on getting:

Attempted to handle event loadedData on [model] while in state rootState.loaded.updated.uncommitted.

A little more context: we were blocking interactivity based on the isLoaded of the model and then, via an input, allowing the user to change a property bound to same model and save. It was perplexing to figure out:

  • we're not allowing UI controls to be enabled until the model is loaded (ok)
  • then the model is loaded (ok)
  • then the user makes changes and saves (ok)
  • then there's a race condition because the model hasn't been loaded yet (what?!?)

For the step of the spec that was failing under Poltergeist, was seeing only this for the state transitioning of the model (via console.log):
saved

In a browser (manually) and under Selenium, I saw:
saved
materializing
saved

Until the problem is fixed, my work-around is to implement a savedCount prop on the controller to count how many times 'saved' for content.stateManager.currentState.name has been seen. Two (or more) seems to be the magic number.

So glad to see this being addressed. We had an acceptance test failing and I took me a while to get to the bottom of why.

In the browser (manually) and under Selenium, functionality was fine. The spec failed under Poltergeist; kept on getting:

Attempted to handle event loadedData on [model] while in state rootState.loaded.updated.uncommitted.

A little more context: we were blocking interactivity based on the isLoaded of the model and then, via an input, allowing the user to change a property bound to same model and save. It was perplexing to figure out:

  • we're not allowing UI controls to be enabled until the model is loaded (ok)
  • then the model is loaded (ok)
  • then the user makes changes and saves (ok)
  • then there's a race condition because the model hasn't been loaded yet (what?!?)

For the step of the spec that was failing under Poltergeist, was seeing only this for the state transitioning of the model (via console.log):
saved

In a browser (manually) and under Selenium, I saw:
saved
materializing
saved

Until the problem is fixed, my work-around is to implement a savedCount prop on the controller to count how many times 'saved' for content.stateManager.currentState.name has been seen. Two (or more) seems to be the magic number.

tomdale added a commit that referenced this pull request May 10, 2013

Merge pull request #827 from mmpestorich/setup-data-without-materiali…
…zing

Setup data without triggering materialization.

@tomdale tomdale merged commit af72965 into emberjs:master May 10, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment