Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#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

     
    4343                //                        Promise
    4444                if(args.data){
    4545                        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){
    4747                        var model;
    4848                        var result = args.store.query(args.query);
    4949                        if(result.then){

Change History (9)

comment:1 Changed 8 years ago by 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?

comment:2 Changed 8 years ago by 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.

comment:3 Changed 8 years ago by 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

comment:4 Changed 8 years ago by rahul

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

comment:5 Changed 8 years ago by 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.

comment:6 Changed 8 years ago by ben hockey

Resolution: fixed
Status: newclosed

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

comment:7 Changed 8 years ago by 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.

comment:8 Changed 8 years ago by 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.

comment:9 Changed 8 years ago by bill

Milestone: tbd1.7
Note: See TracTickets for help on using tickets.