Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13446 closed defect (fixed)

MVC - dot separated declarative ref with implied parent relative binding fails

Reported by: ben hockey Owned by: ben hockey
Priority: high Milestone: 1.7
Component: DojoX MVC Version: 1.7.0b1
Keywords: Cc:
Blocked By: Blocking:

Description

the summary is a mouthful... the short version is, this doesn't work with a parent binding

<div data-dojo-prop="ref:'foo.bar', ..."></div>

i'm expecting this to get parentBinding.foo.bar but instead it's essentially trying to get parentBinding['foo.bar']. subtle difference but not useful :)

the solution is simple and may even help explain the problem better

  • _DataBindingMixin.js

     
    199199                        }else{ // defaults: outermost refs are expressions, nested are relative to parents
    200200                                parentBinding = parentBinding || this._getParentBindingFromDOM();
    201201                                if(parentBinding){
    202                                         binding = parentBinding.get(ref);
     202                                        binding = dojo.getObject(ref, false, parentBinding);
    203203                                }else{
    204204                                        try{
    205205                                                binding = dojo.getObject(ref);

Change History (11)

comment:1 Changed 8 years ago by ben hockey

Component: GeneralDojoX MVC
Owner: set to rahul

comment:2 Changed 8 years ago by rahul

+1, I'm quite sure the proposed fix was the intent ;-)

Unrelated to this ticket, but related to the topic, I've also thought about the pluggability of the ref syntax itself i.e. say if someone wants to come along and use something like JSONPath instead of a dot separated ref expression, could we afford that change to be made more easily (and the related topic of a real dependency graph in the model when relative references are not simple as dot separated property access, but thats truly a digression).

comment:3 Changed 8 years ago by ben hockey

Owner: changed from rahul to ben hockey
Status: newassigned

part of my outline in #13263 may lend itself towards making the ref syntax a little more pluggable. my outline includes _setRefAttr which is dijit's version of a setter for the ref property. any time someone calls widget.set('ref', ref) the _setRefAttr function is called with ref being passed to it. this provides an extension point that would be suitable for various implementations of ref parsing. we could either leave it for subclasses to override the default implementation if they want different behavior (favors programmatic) or we could implement it in such a way that it provided a framework for plugins to interpret ref (favors declarative). some of the date-based widgets have a similar thing to allow various date packages to be used (see datePackage in dijit.CalendarLite?).

comment:4 Changed 8 years ago by ben hockey

Resolution: fixed
Status: assignedclosed

fixed in r25798

comment:5 Changed 8 years ago by ben hockey

Resolution: fixed
Status: closedreopened

comment:6 Changed 8 years ago by Ed Chatelain

Hi Ben, thanks for reopening this. (I am just making this comment to get on the cc list)...

comment:7 Changed 8 years ago by ben hockey

reopening this because ed let me know that r25798 broke cases where a property was trying to be set but the name of the property is an integer rather than a string.

eg

// broken
model.set(1, value);

// vs

//works
model.set("1", value);

the question is at what level do we want to fix this problem?

i could probably change r25798 such that it was dojo.getObject('' + ref, false, parentBinding); and cast ref to a string but i'm not sure if this is the right level to fix this since Stateful::get is documented as needing a string for the property name. this change would make dojo.getObject work but we would still have code trying to do "illegal" things. also, this line of code was just a copy of what's happening a few lines above in the branch that handles declarative rel type refs so that branch would break the same as this if a test covered that as well.

how do you guys think this should be fixed?

comment:8 Changed 8 years ago by ben hockey

hmm... i think i spoke too soon and i'm wrong about the code doing "illegal" things. it could be the right thing to just change that call to getObject so that we cast ref to a string. if there's no objections i'll just make that change. should i change the similar call on line 188?

comment:9 Changed 8 years ago by Ed Chatelain

Looks good to me, all tests ran successfully with both lines updated.

comment:10 Changed 8 years ago by ben hockey

Resolution: fixed
Status: reopenedclosed

(In [25818]) coerce dojo.getObject calls to use strings. fixes #13446

comment:11 Changed 8 years ago by bill

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