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

Add Ember.Deferred mixin which implements Promises/A spec #1406

Merged
merged 1 commit into from Oct 11, 2012

Conversation

Projects
None yet
9 participants
@tchak
Member

tchak commented Sep 26, 2012

No description provided.

@tchak tchak referenced this pull request Sep 26, 2012

Closed

Add Ember.Deferred mixin #1272

@sly7-7

This comment has been minimized.

Show comment
Hide comment
@sly7-7

sly7-7 Sep 26, 2012

Contributor

Is the idea behind this to use this mixin in DS.Model in order to deal with async creation/load of models, and then deferring the routing ?

Contributor

sly7-7 commented Sep 26, 2012

Is the idea behind this to use this mixin in DS.Model in order to deal with async creation/load of models, and then deferring the routing ?

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Sep 26, 2012

Member

@sly7-7 yes, and possibly in DS.Transaction

Member

tchak commented Sep 26, 2012

@sly7-7 yes, and possibly in DS.Transaction

@sly7-7

This comment has been minimized.

Show comment
Hide comment
@sly7-7

sly7-7 Sep 26, 2012

Contributor

This would be great... I'm currently trying to upgrade my app against relationship-improvments branch, and it's really a pain... I don't know if it's usual, but I must constently check if a record is loaded before using it, before adding a child to it etc... I'm facing a lot more issues about not beeing in the right state before modifying records etc... I think this kind of deffered should help me in some ways...

Contributor

sly7-7 commented Sep 26, 2012

This would be great... I'm currently trying to upgrade my app against relationship-improvments branch, and it's really a pain... I don't know if it's usual, but I must constently check if a record is loaded before using it, before adding a child to it etc... I'm facing a lot more issues about not beeing in the right state before modifying records etc... I think this kind of deffered should help me in some ways...

@sly7-7

This comment has been minimized.

Show comment
Hide comment
@sly7-7

sly7-7 Sep 26, 2012

Contributor

@pangratz Yes, I read those resources, but I think this would not be sufficient. At least there should be an other observer on 'didCreate', for just created record. The others issues I have now, do not concern only the routing, but more related to the records lifecycle. With the relationship-improvements branch, I encounter lots of errors like this: Uncaught Error: DS.StateManager:ember2230 could not respond to event becameDirty in state rootState.loaded.updated.inFlight. Perhaps I misuse the new way of create/delete/modify/add children, but I can't figure out what to do. Perhaps our data model is too complicated, with too much relations between each model (like a graph), with lots of hasMany/belongsTo. For now, I'm fighting against ember-data, and have a hard time with it... I hoped it would help me instead... anyway, I will speak with @tomdale, I'm pretty sure he can help me to understand what I'm doing wrong.

Contributor

sly7-7 commented Sep 26, 2012

@pangratz Yes, I read those resources, but I think this would not be sufficient. At least there should be an other observer on 'didCreate', for just created record. The others issues I have now, do not concern only the routing, but more related to the records lifecycle. With the relationship-improvements branch, I encounter lots of errors like this: Uncaught Error: DS.StateManager:ember2230 could not respond to event becameDirty in state rootState.loaded.updated.inFlight. Perhaps I misuse the new way of create/delete/modify/add children, but I can't figure out what to do. Perhaps our data model is too complicated, with too much relations between each model (like a graph), with lots of hasMany/belongsTo. For now, I'm fighting against ember-data, and have a hard time with it... I hoped it would help me instead... anyway, I will speak with @tomdale, I'm pretty sure he can help me to understand what I'm doing wrong.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Sep 26, 2012

Member

@sly7-7 relationship-improvements is not in master for a reason. It is unstable. I heard a lot of reports about lifecycle events. There is even some code missing emberjs/data#378

Member

tchak commented Sep 26, 2012

@sly7-7 relationship-improvements is not in master for a reason. It is unstable. I heard a lot of reports about lifecycle events. There is even some code missing emberjs/data#378

@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Sep 26, 2012

Member

relationship-improvements does have a didCreate callback on models, but it's called before the returned json is applied to the record, so the record lacks useful things like id. I talked to @wycats to confirm that was unintended and that many of the lifecycle callbacks now behave wonky. It'll be fixed soon.

For now, you can override your store's didSaveRecord method and trigger your own callback:

  didSaveRecord(record, json){
    this._super(record, json);
    record.trigger('didCreateForRealz');
  }

This commit will be most useful when a transaction includes multiple records that have to save via multiple requests and you need notification of when all the requests have completed.

Member

trek commented Sep 26, 2012

relationship-improvements does have a didCreate callback on models, but it's called before the returned json is applied to the record, so the record lacks useful things like id. I talked to @wycats to confirm that was unintended and that many of the lifecycle callbacks now behave wonky. It'll be fixed soon.

For now, you can override your store's didSaveRecord method and trigger your own callback:

  didSaveRecord(record, json){
    this._super(record, json);
    record.trigger('didCreateForRealz');
  }

This commit will be most useful when a transaction includes multiple records that have to save via multiple requests and you need notification of when all the requests have completed.

@sly7-7

This comment has been minimized.

Show comment
Hide comment
@sly7-7

sly7-7 Sep 26, 2012

Contributor

Yep I realized this missing code, I've implemented this. If I tried to switch to this branch, it's because with master, all does not work, and Tom suggest me to try the new branch. Anyway, I think these are complex problems, so complex to resolve. The thing is I'm not yet good enough to tackle them by myself, and so not comfortable to try to hack ember-data code in order to make things work.

Contributor

sly7-7 commented Sep 26, 2012

Yep I realized this missing code, I've implemented this. If I tried to switch to this branch, it's because with master, all does not work, and Tom suggest me to try the new branch. Anyway, I think these are complex problems, so complex to resolve. The thing is I'm not yet good enough to tackle them by myself, and so not comfortable to try to hack ember-data code in order to make things work.

@sly7-7

This comment has been minimized.

Show comment
Hide comment
@sly7-7

sly7-7 Sep 26, 2012

Contributor

@trek thank you for the tip :)

Contributor

sly7-7 commented Sep 26, 2012

@trek thank you for the tip :)

@krisselden

View changes

Show outdated Hide outdated packages/ember-runtime/lib/mixins/deferred.js
@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Sep 28, 2012

Member

@kselden Thank's for the tip! I changed my implementation.

Member

tchak commented Sep 28, 2012

@kselden Thank's for the tip! I changed my implementation.

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Oct 3, 2012

Member

Looks good to me, @wycats @tomdale thoughts?

Member

krisselden commented Oct 3, 2012

Looks good to me, @wycats @tomdale thoughts?

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Oct 11, 2012

Member

LGTM

Member

ebryn commented Oct 11, 2012

LGTM

ebryn added a commit that referenced this pull request Oct 11, 2012

Merge pull request #1406 from tchak/deferred-then
Add Ember.Deferred mixin which implements Promises/A spec

@ebryn ebryn merged commit f7ac080 into emberjs:master Oct 11, 2012

1 check passed

default The Travis build passed
Details
@suarasaur

This comment has been minimized.

Show comment
Hide comment

suarasaur commented Oct 14, 2012

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Oct 14, 2012

Member

@jden I think you have a good point. Let me know when you have some progress on the test suite, and I will happily look in to fixing or api.

Member

tchak commented Oct 14, 2012

@jden I think you have a good point. Let me know when you have some progress on the test suite, and I will happily look in to fixing or api.

@suarasaur

This comment has been minimized.

Show comment
Hide comment
@suarasaur

suarasaur Oct 14, 2012

@tchak - just to clarify, that's @domenic's gist. Paging @domenic to the thread...

suarasaur commented Oct 14, 2012

@tchak - just to clarify, that's @domenic's gist. Paging @domenic to the thread...

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Oct 14, 2012

Member

Right! @domenic could you let us know if you have some progress on the tests?

Member

tchak commented Oct 14, 2012

Right! @domenic could you let us know if you have some progress on the tests?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 14, 2012

@tchak working on them now, ETA 30 minutes.

domenic commented Oct 14, 2012

@tchak working on them now, ETA 30 minutes.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Oct 14, 2012

Member

Wow! I will probably be asleep in 30min 😄 but I will look in to it tomorrow!
Thank's again for clarifications! Looking forward to fix it!

Member

tchak commented Oct 14, 2012

Wow! I will probably be asleep in 30min 😄 but I will look in to it tomorrow!
Thank's again for clarifications! Looking forward to fix it!

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 15, 2012

OK, here we go! https://github.com/domenic/promise-tests

It's not the easiest thing to run outside of Node.js, so if that's a problem for you, let me know and I can find a way to make it work in browsers.

domenic commented Oct 15, 2012

OK, here we go! https://github.com/domenic/promise-tests

It's not the easiest thing to run outside of Node.js, so if that's a problem for you, let me know and I can find a way to make it work in browsers.

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Oct 17, 2012

Member

For those following at home, or linking through from Bradley Priest's recap, this will likely be superseded by #1459

Member

lukemelia commented Oct 17, 2012

For those following at home, or linking through from Bradley Priest's recap, this will likely be superseded by #1459

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