Opened 6 years ago

Closed 5 years ago

#16693 closed feature (fixed)

support for ES5 getters/setters

Reported by: bill Owned by: bill
Priority: undecided Milestone: 2.0
Component: Dijit Version: 1.8.3
Keywords: Cc: ben hockey, Ed Chatelain
Blocked By: Blocking:

Description (last modified by bill)

Ticket to prospectively add support for ES5 getters/setters to Dijit for 2.0, plus doing some preparatory (non backwards-compatibility breaking) work in 1.9.

The preparatory work includes making sure all custom setters store their values by calling this._set(...) instead of directly calling this.foo = bar, since the latter causes an infinite recursion. This is actually leftover work from #11251.

In addition, for getters _getXYZAttr() that reference this.XYZ, possibly add a level of indirection. I think eventually they will access this.props.XYZ instead. Alternately, there's probably some overuse of custom getters, which should be removed [and replaced by custom setters], for example !ValidationTextBox::_getPatternAttr().

Note that it's likely not practical to setup custom setters/getters for every possible attribute. I just checked http://www.w3.org/TR/wai-aria/states_and_properties, there are a lot of aria attributes alone. These attributes could still be handled if passed as arguments to the widget constructor, or specified in markup, by calling the old auto-mapping code in set().

Change History (17)

comment:1 Changed 6 years ago by bill

Description: modified (diff)
Milestone: tbd2.0
Status: newassigned

Some changes already checked in for this include [30567] and [30569].

comment:2 Changed 6 years ago by bill

In [30570]:

Call this._set("xyz", ...) instead of directly setting this.xyz = ..., both so watch() works and for future support of ES5 native setters/getters. Refs #11251, #16693 !strict.

comment:43 Changed 6 years ago by bill

In [30571]:

Introduce _WidgetBase._get() method for accessing properties stored by this._set(), i.e. to get the value of properties with custom setters. Currently just returns this[value], but will do more in 2.0. Refs #16693 !strict.

comment:44 Changed 6 years ago by bill

In [30576]:

If ValidationTextBox.pattern is a function, then ValidationTextBox.get("pattern") should return that function, rather than the result of the function. Also, _setRegExpGenAttr() should use this._set() to save the value. Refs #13433, #16693 !strict.

comment:7 Changed 6 years ago by bill

In [30577]:

use this._set() to save value in custom setter, and this._get() to get it, refs #11251, #16693 !strict.

comment:44 Changed 6 years ago by bill

In [30578]:

Use get() to access a widget's property, refs #16693 !strict

comment:45 Changed 6 years ago by bill

In [30579]:

Use lang.delegate() rather than lang.mixin(), both for efficiency and to avoid spuriously triggering custom getters in "this". Refs #10362, #11889, #16693 !strict.

comment:4 Changed 6 years ago by bill

In [30580]:

More fixup to use this._set() and this._get(), refs #16693 !strict.

comment:5 Changed 6 years ago by bill

In [30598]:

Move widget metadata collection from _applyAttributes() and _onMap() to new _introspect() method, which is hopefully faster since it only scans the prototype properties once. Also, collect list of all properties with custom getters or setters, and pre-generate custom setter functions for custom setters that are strings or objects (ex: _setFooAttr: "focusNode"). Refs #16693 !strict.

comment:6 Changed 6 years ago by bill

In [30599]:

Replace _attrToDom() method with domSetter() method that generates a custom setter function to map a value to a specified DOMNode's attribute, innerHTML, etc. Use domSetter() when generating this._constructor.props{} hash.

In the future, for performance, probably the existing non-function setters in prototypes (ex: _setLabelAttr: "labelNode") should be changed to use domSetter() directly, so that the setter in the superclass can be reused by the subclasses. Alternately, implement caching so domSetter() returns a previously created setter when possible.

Refs #16693 !strict.

comment:7 Changed 6 years ago by ben hockey

Cc: ben hockey added

comment:8 Changed 6 years ago by Eric Durocher

[30598] is breaking dojox/mobile/IconContainer. This class extends itself dynamically with the _EditableIconMixin when the editable attribute is true. _EditableIconMixin defines a custom setter _setEditableAttr, which must be called to initialize touch handlers, etc. But, after 30958 and the introduction of _introspect(), the setter is never called. One problem seems to be that _introspect() is called before _EditableIconMixin is added. But I also tried to call again _introspect() (after clearing this.constructor._props), but the setter is still not found...

comment:9 Changed 6 years ago by bill

Eric added workaround code for dojox/mobile, see #16737. Note that before my change, adding a _setEditableAttr() method to the instance sort-of works, except that _applyAttributes() usually won't automatically call it on creation, even if editable has a non-falsy value (see #7381), because _applyAttributes() collects meta-data about the prototype on its first call.

Dojox/mvc is also getting failures though, see #16738. Partly because _atBindingMixin.js directly accesses this.constructor._setterAttrs.

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

comment:10 Changed 6 years ago by Ed Chatelain

Cc: Ed Chatelain added

comment:11 Changed 6 years ago by bill

In [30659]:

Rollback [30599] and part of [30598]. They break code like dojox/mvc that dynamically adds setters to an instance, and also have a problem where defining a custom getter disables the code that automatically maps widget properties to DOMNode attributes (failing in dojox/mvc/Element.js for value property). As in 1.8, the dynamically added setters won't be automatically called on widget creation unless the property is specified as a parameter to the widget constructor. Refs #16693 and fixes #16738 !strict.

Also rolling back workaround code from [30639], refs #16737.

For 2.0 I'll probably have an API to add properties (with getters or setters) to an instance, and either remove or rearchitect how properties are automatically mapped to DOMNode attributes.

comment:12 Changed 6 years ago by bill

Description: modified (diff)

comment:13 Changed 5 years ago by bill

Resolution: fixed
Status: assignedclosed

Implemented in our 2.0 prototype, so closing this ticket.

Note: See TracTickets for help on using tickets.