The Problem
Suppose you are writing code for some DS.Model using computedPromise
IssueThread = DS.Model.extend
comments: DS.hasMany "comments", async: true
okToMerge: computedPromise "comments.[]", ->
@get("comments")
.then (comments) ->
comments.find (comment) ->
comment.get("speaker") is "rwjblue" &&
comment.get("comment") is "lgtm"Aside from the infinite loop notification problem as mentioned in #132 (comment)
There is another issue regarding different instances of an object sharing the same result state.
The Reason
From the code at:
https://github.com/cibernox/ember-cpm/blob/master/addon/macros/computed-promise.js#L41
We see that both pendingPromise and result are declared outside the computed property, which means if you have something like:
issue1 = @store.find "issue-thread", 1
issue2 = @store.find "issue-thread", 2
issue1.get("okToMerge") # Call to start the promise
waitUntilResolve()
issue2.get("okToMerge") === issue1.get("okToMerge")That is, even though we have two distinct instances of the IssueThread, they both share the result state... which is definitely a bug
My Humble Suggestion
Please consider just putting pendingPromise and result directly into the object instance, i.e.:
return computed(...dependentKeys, function(key) {
if (!this[key + "PendingPromise"]) {
const promise = fn.call(this);
this[key + "PendingPromise"] = true;
Ember.RSVP.resolve(promise)
.then((promiseResult) => {
this[key + "Result"] = promiseResult;
pendingPromise = false;
this.notifyPropertyChange(key);
});
}
return this[key + "Result"];
});Yes, this feels dirty, but this way, state is properly tied to each object instance, not to some closure shared by all instances of a factory. And we could always put in some assert to ensure the additional property names aren't colliding, and tell off the user if they are.
The Problem
Suppose you are writing code for some DS.Model using
computedPromiseAside from the infinite loop notification problem as mentioned in #132 (comment)
There is another issue regarding different instances of an object sharing the same result state.
The Reason
From the code at:
https://github.com/cibernox/ember-cpm/blob/master/addon/macros/computed-promise.js#L41
We see that both
pendingPromiseandresultare declared outside the computed property, which means if you have something like:That is, even though we have two distinct instances of the
IssueThread, they both share theresultstate... which is definitely a bugMy Humble Suggestion
Please consider just putting
pendingPromiseandresultdirectly into the object instance, i.e.:Yes, this feels dirty, but this way, state is properly tied to each object instance, not to some closure shared by all instances of a factory. And we could always put in some
assertto ensure the additional property names aren't colliding, and tell off the user if they are.