MVC - typo in newStatefulModel

[ben hockey]

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
                 if(args.data){
                         return new StatefulModel({ data : args.data });
-                }else if(args.store && typeof lang.isFunction(args.store.query)){
+                }else if(args.store && args.store.query){
                         var model;
                         var result = args.store.query(args.query);
                         if(result.then){

[rahul]

I suspect it'd be adequate whereas the existing one is more defensive (rathe than a typo). Any standout reason for suggesting this change?

[ben hockey]

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.

[ben hockey]

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

[rahul]

Got it, typeof is indeed cruft from converting to lang.isFunction, though lang.isFunction can stay IMO.

[ben hockey]

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.

[ben hockey]

(In [25837]) fix typo in dojox/mvc/_base.js fixes #13480 !strict

[Ed Chatelain]

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.

[ben hockey]

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.