Opened 7 years ago
Closed 7 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 7 years ago by
comment:2 Changed 7 years ago by
Owner: | set to haysmark |
---|---|
Status: | new → pending |
comment:3 Changed 7 years ago by
Status: | pending → new |
---|
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 7 years ago by
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:6 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:7 Changed 7 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 7 years ago by
Resolution: | → wontfix |
---|---|
Status: | reopened → closed |
comment:9 Changed 7 years ago by
Milestone: | tbd → 1.7.6 |
---|---|
Resolution: | wontfix |
Status: | closed → reopened |
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.
comment:10 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
master: https://github.com/dojo/docs/commit/582b19cc3ffde84b65b56d5d191d5aa3a1aea8cb 1.9: https://github.com/dojo/docs/commit/8e418d210a7eff6bbe26c2ee26df01ecc1a10fd6 1.8: https://github.com/dojo/docs/commit/193b7e28043944a013dcc7aaddcaf930bdc48527 1.7: https://github.com/dojo/docs/commit/1c51cc9d0f06cb49f3acd1a18fc5a448b6322756
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.