Opened 6 years ago

Closed 6 years ago

#17395 closed defect (fixed)

dojo/store/Observable does not handle stores that return promises from getIdentity()

Reported by: haysmark Owned by: haysmark
Priority: undecided Milestone: 1.7.6
Component: Data Version: 1.9.1
Keywords: Cc:
Blocked By: Blocking:

Description

As per the dojo/store spec, any store function can return a promise, including getIdentity().

dojo/store/Observable does not handle this case.  It assumes that getIdentity() directly returns a key, rather than a promise containing that key. While it is true that Memory stores return the value, in general, the Observable API should not assume this detail.

There are two relevant snippets where getIdentity is used:

// remove the old one
for(i = 0, l = resultsArray.length; i < l; i++){
	var object = resultsArray[i];
	if(store.getIdentity(object) == existingId){

One way to correct the above code would be to add a new promise like "oldOneFound" and add the rest of the code as its "after" action. Simply add an else branch to the above if statement to listen in on the identity promises as they are encountered. Whenever the result is found, whether returned directly by getIdentity or by a promise, fire the oldOneFound promise.

whenFinished("put", function(object){
	store.notify(object, store.getIdentity(object));
});

It would be straightforward to rewrite this to check for a promise.

Change History (10)

comment:1 Changed 6 years ago by Kris Zyp

It doesn't seem worthwhile to support asynchronous getIdentity. How does the benefits of supporting this outweigh the non-negligible costs? I have updated the docs at http://livedocs.dojotoolkit.org/dojo/store#methods to indicate that getIdentity should always be synchronous.

comment:2 Changed 6 years ago by ben hockey

Owner: set to haysmark
Status: newpending

comment:3 Changed 6 years ago by haysmark

Status: pendingnew

I'm fine with kzyp's doc fix. Do we have the technical capacity to backport doc fixes? getIdentity always needed to be synchronous but we never spelled it out in past versions.

comment:4 Changed 6 years ago by bill

Sure, doc fixes are backported just like code fixes. The repository is https://github.com/dojo/docs. After that (eventually) you need to build and deploy to the website, which Kitson does. Can you backport the doc change and then close this ticket?

comment:5 Changed 6 years ago by haysmark

Status: newassigned

Thanks Bill, I'll take care of it.

comment:6 Changed 6 years ago by haysmark

Resolution: fixed
Status: assignedclosed

comment:7 Changed 6 years ago by bill

Resolution: fixed
Status: closedreopened

Guess we should close this as invalid or something; otherwise I don't know what version to set for the milestone it was "fixed" in.

comment:8 Changed 6 years ago by bill

Resolution: wontfix
Status: reopenedclosed

comment:9 Changed 6 years ago by haysmark

Milestone: tbd1.7.6
Resolution: wontfix
Status: closedreopened

Well technically the ticket has commits in its name going back to 1.7, but alas neither kzyp nor I followed the commit guidelines and put the "Fixes #17395" in the commits. I'll link all of the commits then close as fixed.

Note: See TracTickets for help on using tickets.