#13480 closed defect (fixed)
MVC - typo in newStatefulModel
Reported by: | ben hockey | Owned by: | rahul |
---|---|---|---|
Priority: | high | Milestone: | 1.7 |
Component: | DojoX MVC | Version: | 1.7.0b1 |
Keywords: | Cc: | Ed Chatelain | |
Blocked By: | Blocking: |
Description
there looks to be a typo in the newStatefulModel
factory method.
see http://bugs.dojotoolkit.org/browser/dojox/trunk/mvc/_base.js?rev=25836#L46
i'm thinking the following patch would be adequate (if args.store.query exists then it assumes its a function)
-
_base.js
43 43 // Promise 44 44 if(args.data){ 45 45 return new StatefulModel({ data : args.data }); 46 }else if(args.store && typeof lang.isFunction(args.store.query)){46 }else if(args.store && args.store.query){ 47 47 var model; 48 48 var result = args.store.query(args.query); 49 49 if(result.then){
Change History (9)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
look closely... my change has 2 parts - removes a typo (lingering typeof
) and becomes less defensive. the typo needs to go and the less defensive approach is only a matter of preference. i just did what i prefer (and noticed that a few lines later, there is a test for result.then
which does not test if its a function so felt it wasn't completely out of place) but i'm not adamant about that change.
comment:3 Changed 10 years ago by
in case its not clear:
typeof lang.isFunction(whatever)
-> typeof true|false
-> "boolean"
if (args.store && "boolean")
is effectively what's happening with that lingering typeof
comment:4 Changed 10 years ago by
Got it, typeof is indeed cruft from converting to lang.isFunction, though lang.isFunction can stay IMO.
comment:5 Changed 10 years ago by
ok np, i'll commit. it won't be tested because i still can't get the mvc test suite to run but i'm fairly certain the fault is some kind of peculiarity with the test runner and not the tests themselves. i'll close the ticket with the commit but feel free to contact me directly if there are any issues.
comment:6 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:7 Changed 10 years ago by
I just finished running tests for both }else if(args.store && args.store.query){ and }else if(args.store && lang.isFunction(args.store.query)){
Both were fine, of course I did not have a failing testcase to see this problem so not surprising that the fix did not cause any problems.
thanks for the Cc: on this one.
comment:8 Changed 10 years ago by
ed, thanks for testing the changes. there wasn't really a failing case since we would have to invent one to make this break - it would need to use a store without a query function or some other strange thing like that.
i'll try to remember to include you on the cc of any mvc tickets as i open them.
comment:9 Changed 9 years ago by
Milestone: | tbd → 1.7 |
---|
I suspect it'd be adequate whereas the existing one is more defensive (rathe than a typo). Any standout reason for suggesting this change?