Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16080 closed defect (fixed)

params initialized in postMixInProperties are lost in postCreate

Reported by: Irfan Ahmed Owned by: bill
Priority: undecided Milestone: 1.8.1
Component: Dijit Version: 1.8.0
Keywords: Cc: cjolif
Blocked By: Blocking:

Description

This is what I have done.

Declare a custom widget deriving from _WidgetBase, _TemplatedMixin The widget has a set of parameters. Now in postMixInProperties I check the parameter values and if these are not provided or undefined (could have undefined value as these are programmatically created) then default or calculated values are set in these params.

The domNode is created and the DOM has the correct initialized values set in postMixInProperties. However the values of these params in postCreate is back to what it was when the postMixInProperties was called. In 1.7.3, the postCreate retains the new values set in postMixInProperties: however in 1.8.0, this behaviour is not maintained. Is this a regression or am I doing something I should not be doing.

Attachments (4)

ParamsBug.html (2.2 KB) - added by Irfan Ahmed 7 years ago.
It contains a test case. The values in the postCreate should have been the new values set in postMixInProperties
output-173.png (38.4 KB) - added by Irfan Ahmed 7 years ago.
Output in Dojo 1.7.3
output-180.png (37.6 KB) - added by Irfan Ahmed 7 years ago.
Output in Dojo 1.8.0
16080-patch.txt (272 bytes) - added by Irfan Ahmed 7 years ago.
Patch for using this[param] instead of this.params[param]

Download all attachments as: .zip

Change History (18)

Changed 7 years ago by Irfan Ahmed

Attachment: ParamsBug.html added

It contains a test case. The values in the postCreate should have been the new values set in postMixInProperties

Changed 7 years ago by Irfan Ahmed

Attachment: output-173.png added

Output in Dojo 1.7.3

Changed 7 years ago by Irfan Ahmed

Attachment: output-180.png added

Output in Dojo 1.8.0

comment:1 Changed 7 years ago by cjolif

This is probably related to the first item in the release notes here:

http://livedocs.dojotoolkit.org/releasenotes/1.8#widgetbase

A workaround is to modify your postMixInProperties to something like the following (there might be better ones):

postMixInProperties: function() {
  if(!this.name) {
	this.name = "Something";
	if(this.params){
  	  this.params.name = "Something";
	}
  }
  if(!this.age || (this.age < 0)) {
        this.age = 16;
	if(this.params){
	  this.params.age = 16;
	}
   }
}

I would personally go a bit further and modify the widget to have "Something" and 16 as the default values for name & age as they seem to be the real default values anyway? And not pass undefined or null in ctor param when I don't have a valid values for this and let the widget use its own.

Last edited 7 years ago by cjolif (previous) (diff)

comment:2 Changed 7 years ago by Irfan Ahmed

This is not a solution. The postMixInProperties was uptil now the function where you could manipulate the params set in the widget. Now the param values are being reassigned. Changing the params is not something that should be done in the postMixInProperties but in postscript. Also once the params in "this" have been assigned, IMHO these should be maintained and not reverted back.

Why should the params be reverted back to their original values once I have set new values in the postMixInProperties. Hence I feel this is a bug. I have attached a patch. Instead of setting the params in "this" to their original values, should we not set these to the values specified in "this" in postMixInProperties. We anyways mixin the params to the object in create and from then onwards we should not use the value in params, rather the value in the object.

Changed 7 years ago by Irfan Ahmed

Attachment: 16080-patch.txt added

Patch for using this[param] instead of this.params[param]

comment:3 in reply to:  2 Changed 7 years ago by cjolif

Replying to irfan0719:

This is not a solution. The postMixInProperties was uptil now the function where you could manipulate the params set in the widget. Now the param values are being reassigned. Changing the params is not something that should be done in the postMixInProperties but in postscript. Also once the params in "this" have been assigned, IMHO these should be maintained and not reverted back.

Well I did not say this was the long term solution I was just providing a workaround so you are not blocked. I guess Bill will look into the details...

comment:4 Changed 7 years ago by bill

Resolution: wontfix
Status: newclosed

This problem is fallout from [27308], which fixed #14341. @irfan0719's patch rolls back [27308], thus reintroducing the problem listed in #14341.

I agree that the new 1.8 behavior is problematic when a widget tries to adjust a parameter value "foo" in postMixInProperties() by setting this.foo. But it seems like we need to make a compromise one way or the other, so I'm not inclined to revert [27308].

Also, as @cjolif said, @irfan0719's test case is strange, as it should be written more simply, without a postMixInProperties() method at all, like:

declare("MyTestWidget", [_WidgetBase, _TemplatedMixin], {
     name: "Something",
     age: 16,
     _setAgeAttr: function(val){
           this._set("age", val < 0 ? 16 : val);
     }
     ...   

I do agree that postMixInProperties() seems rather meaningless now, and should probably be removed for 2.0, since the tasks of "mixing in properties" and "calling custom setters" are basically the same thing.

I'm going to mark this as wontfix for now... will reconsider if more people get hit by this.

Last edited 7 years ago by bill (previous) (diff)

comment:5 Changed 7 years ago by Irfan Ahmed

The problem is that some this code is their since 1.5 where the behaviour was as I had described earlier. The behaviour is broken in 1.8 however. It does break backward compatibility.

comment:6 Changed 7 years ago by ben hockey

Resolution: wontfix
Status: closedreopened

i'm with irfan0719 - postMixInProperties has always been a place to make adjustments to the values mixed in from the constructor and from that point on, widgets rely on the values as they are after postMixInProperties has been applied. i have relied on this for many versions of dojo and this change in behavior to propagate the values passed to the constructor is quite a significant change in behavior (even if the previous behavior isn't normal to most people).

i feel it would be better not to support #14341 than to break backwards compatibility on something which has existed for a very long time.

comment:7 Changed 7 years ago by bill

Cc: cjolif added

Yes, understood (which is why it's documented in http://dojotoolkit.org/reference-guide/1.8/releasenotes/1.8.html#widgetbase).

Christophe, what do you think?

comment:8 Changed 7 years ago by Irfan Ahmed

Actually I have edited this comment to say that postMixInProperties is also a place where you can change the values of attributes based on some other attributes. Or it is also a place where you can create a value based on the values of some input variables. For Eg: In continuation of the example, lets say I create a label with the value as "this.name, this.age" which I have as ${label} in the template. I set the value of this.label in the postMixInProperties.

Even if I had the _setNameAttr and _setAgeAttr, these would be called only after postMixInProps and postMixInProps will have to set the label as "undefined:undefined". I know that the suggestion would be to do it in postCreate, but that means that I have to create either an attachPoint and call node.innerHTML or create an attributeMap and call setter for that. This all means more code to write especially for people who already have create large scale web applications using the above.

Last edited 7 years ago by Irfan Ahmed (previous) (diff)

comment:9 in reply to:  4 Changed 7 years ago by Irfan Ahmed

Replying to bill:

This problem is fallout from [27308], which fixed #14341. @irfan0719's patch rolls back [27308], thus reintroducing the problem listed in #14341.

I agree that the new 1.8 behavior is problematic when a widget tries to adjust a parameter value "foo" in postMixInProperties() by setting this.foo. But it seems like we need to make a compromise one way or the other, so I'm not inclined to revert [27308].

Also, as @cjolif said, @irfan0719's test case is strange, as it should be written more simply, without a postMixInProperties() method at all, like:

declare("MyTestWidget", [_WidgetBase, _TemplatedMixin], {
     name: "Something",
     age: 16,
     _setAgeAttr: function(val){
           this._set("age", val < 0 ? 16 : val);
     }
     ...   

I do agree that postMixInProperties() seems rather meaningless now, and should probably be removed for 2.0, since the tasks of "mixing in properties" and "calling custom setters" are basically the same thing.

I'm going to mark this as wontfix for now... will reconsider if more people get hit by this.

The above code will work for a markup declaration but will not work for a widget that is declared programmatically with value of age as undefined. I have an alert widget that can be called from any where in the code and some of its fields can be undefined. The setter for such fields will not be called until after postMixInProperties and since the field is used in the template, the code will fail. This is a common use case in programmatic creation of widgets.

comment:10 Changed 7 years ago by bill

Yeah, I agree that things are tricky since buildRendering() is called before the custom setters are called. As you said, my code suggestion above won't handle

new MyTestWidget({age: undefined})

if age is accessed in the template, or anywhere in buildRendering().

Conversely, your code won't handle:

myTestWidget.set("age", undefined);

For 2.0 we could call the custom setters before buildRendering(), but that creates a new complication, that setters must branch depending on whether or not the DOM exists.

comment:11 in reply to:  7 Changed 7 years ago by cjolif

Replying to bill:

Yes, understood (which is why it's documented in http://dojotoolkit.org/reference-guide/1.8/releasenotes/1.8.html#widgetbase).

Christophe, what do you think?

Maybe we could imagine a fix that would keep #14341 without causing the #16080 regression?

Something like:

		if(params){
			this.params = params;
			lang.mixin(this, params);
		}
		this.postMixInProperties();
		for(var key in params){
			this.params[key] = this[key];
		}

(just brainstorming)

comment:12 Changed 7 years ago by bill

In [29760]:

Rework fix to #14341 so that properties can still be modified in postMixInProperties(). Refs #16080, #14341 on trunk !strict.

comment:15 Changed 7 years ago by bill

Resolution: fixed
Status: reopenedclosed

In [29761]:

Rework fix to #14341 so that properties can still be modified in postMixInProperties(). Fixes #16080, #14341 on 1.8 branch !strict.

comment:16 Changed 7 years ago by bill

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