#16480 closed defect (fixed)
Observable async stores could not be used with StoreRefController
Reported by: | Johnathon Harris | Owned by: | Ed Chatelain |
---|---|---|---|
Priority: | undecided | Milestone: | 1.8.4 |
Component: | DojoX MVC | Version: | 1.8.2rc1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
The StoreRefController? does some copying of attributes at the end of queryStore so that Observable results are maintained. Unfortunately this does not work if the result it modifies is a Promise (e.g. if you are using an Observable JsonRest? store).
The error will be that the results eventually generated will no longer have the observe method.
A simple fix is to move the logic which copies the Observable attributes into the callback for dojo/when invoked on the promised results.
Attachments (3)
Change History (25)
Changed 8 years ago by
Attachment: | dojo-1.8.2_StoreRefController_ObservableAsyncStoreFix_ticket16480.patch added |
---|
comment:2 Changed 8 years ago by
jmharris do you have a doh test case as well, so that we can check regressions in the future?
Thanks.
comment:3 Changed 8 years ago by
Thanks for the patch. I am on vacation until early January so I will not be able to work on this until I return.
comment:4 Changed 8 years ago by
Looks that this will be fixed by [30552], so we can close this ticket?
comment:5 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Thanks asudoh, I will close it as fixed, it was fixed by #16656.
comment:6 Changed 8 years ago by
Milestone: | tbd → 1.9 |
---|
comment:7 Changed 8 years ago by
Hi, the patch for Ticket #16656 does not address the problem I reported. Can this ticket be re-opened?
Will the patch I provided not be applied until a doh test case is provided as requested by cjolif?
comment:8 Changed 8 years ago by
If a testcase had been provided we could have used the test to tell if the patch for #16656 actually fixed it or not. So if you can provide the test we will take another look, and see if it should be reopened.
Changed 8 years ago by
Attachment: | ticket16480-testcase.patch added |
---|
comment:9 Changed 8 years ago by
Thanks edchat, please see newly attached test case.
observeQueryResults passes as you would expect.
observeDeferredQueryResults fails and shows the problem I reported regarding async stores.
Applying the dojox/mvc/StoreRefController patch originally submitted will allow observeDeferredQueryResults to pass.
Applying the patch for ticket #16656 makes no difference to the test case.
comment:10 Changed 8 years ago by
I found a bug in EditStoreRefController? not doing decoration for Obsrevable, which is fixed in: https://github.com/edchat/dojox_mvc/pull/77
Given dojo/store/Observable:52-61 tells us that it sets observe()
directly to query()
result no matter it's promise or value, what application needs to do is calling observe()
of what queryStore()
returns, instead of trying to call observe()
of the resolved value.
comment:11 Changed 8 years ago by
Hi asudoh, your patch does not appear to resolve the problem I'm reporting (although I'm not completely sure if you were claiming it does).
I have provided a test case and bug fix patch for this problem- is there anything outstanding?
comment:12 Changed 8 years ago by
Hi jmharris, if this makes it more clear: You cannot expect the resolved value (in other words, that data coming from server/store) always has observe()
method. observe()
method is attached whichever queryStore()
returns, either promise or value. Please refer to my newly added test case how to use observe()
with queryStore()
returning a promise.
comment:13 Changed 8 years ago by
Ok, you are saying that it is the expected behaviour that if I use Observable with a store I get observe() on the results regardless of whether the store defers results or not.. but when I use that store with a StoreRefController? it's fine that no longer works?
That seems odd to me- especially as StoreRefController? specifically has code to copy attributes injected by Observable (which it specifically mentions) over. I'm only suggesting that existing code be moved.
comment:14 Changed 8 years ago by
My newly added test case shows that store returning promise does work with StoreRefController?. There was a bug store returning promise does not work with EditStoreRefController?, which I fixed in my recent commit. I guess what you hit is the latter as your test case refers to EditStoreRefListController?, which is an inheritance of EditStoreRefController?.
Though moving the decoration code to when()
callback may improve things in some scenarios, as far as I see it delays observe()
being set if store returns a promise. Given dojo/store/Observable sets observe()
to what query()
returns right then, I chose to do the same thing.
comment:15 Changed 8 years ago by
Thank you for your insight into this problem asudoh, but EditStoreRefController? and EditStoreRefListController? are inconsequential- my test case only uses those because I've extended some existing test code for the StoreRefController?. I will attach a revised test case to demonstrate this.
You say a 'store returning promise does work with StoreRefController?'. If by 'work' you mean the observe function is available in the results, then no, it does not work. Since this problem can be demonstrated with the plain StoreRefController? I fail to see how your patches to EditStoreRefController? are relevant.
The code in StoreRefController? which tries to copy values onto the result specifically to support dojo/store/Observable (at the end of queryStore) does not work if the store returns a Promise- the observe function cannot be set on a Promise object (running on Chrome 25 at least)!
If you don't want to set observe() on the results during when(), and its demonstrable that they cannot be set before, how would this work otherwise?
Changed 8 years ago by
Attachment: | ticket16480-testcase-StoreRefController.patch added |
---|
Demonstrates that StoreRefController?? fails to make observe() available if the store returns a Promise, even after using dojo/when. Note that before using when() the Promise returned doesn't have observable() either.
comment:16 Changed 8 years ago by
Just attached a refined version of the test case which uses StoreRefController? only. Note that it now demonstrates the Promise object returned does /not/ have the observe() function.
If you debug the code in StoreRefController?.queryStore you can see that while it attempts to copy the functions that Observable added to the Promise, they will not be available.
What more can I do to help here? Do you require more evidence that this is a problem, or do you accept that it is a problem but disagree with the proposed solution?
comment:17 Changed 8 years ago by
If by 'work' you mean the observe function is available in the results
No, (to reiterate my early comment to this ticket) for async case the observe()
function should be in the promise, instead of in the resolved value (results). Please refer to My newly added test case for how JsonRest/Observable? combination should be used with StoreRefController?.
comment:18 Changed 8 years ago by
Thank you for your continued attention to this ticket asudoh, now I can understand where you are coming from. So as I see it the facts are:
- It was not possible to get observe() from a store that returns a Promise in dojo 1.8.3.
- To demonstate this your recently added test cases fail (both observableAsyncStore and observeJsonRest) when applied to the current dojo 1.8.3 release with no other files patched.
- For observableAsyncStore to pass, it requires EditStoreRefController?.js to be patched: https://github.com/asudoh/dojox_mvc/commit/efcd2a14c18e91e693ca86b6c0f65dc6f9d84fdd
- For observeJsonRest to pass, it requires StoreRefController?.js to be patched: http://bugs.dojotoolkit.org/changeset/30552/dojo
- Developers are expected to use observe() on the object returned by queryStore, regardless of whether that is a result set or not, not anything ultimately returned by dojo/when.
While we might disagree on the approach to fixing the problem, it is now clear we agreed this was a problem and there is some solution.
comment:19 Changed 8 years ago by
Thanks jmharris and asudoh, I will backport those fixes to 1.8 as soon as I can.
comment:22 Changed 8 years ago by
Milestone: | 1.9 → 1.8.4 |
---|
Fix for this ticket.