#18768 closed enhancement (fixed)
remove widgetsInTemplate property and instead detect automatically
Reported by: | paulrutter | Owned by: | bill |
---|---|---|---|
Priority: | undecided | Milestone: | 1.11 |
Component: | Dijit | Version: | 1.10.4 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
As stated on https://dojotoolkit.org/documentation/tutorials/1.10/templated/, the _WidgetsInTemplateMixin should only be mixed in when needed, to prevent a performance penalty.
Although this is good advice, it often happens that the mixin is used by default (copied from another widget), even though no widgets are declared in the template. This is done to prevent a hard to debug situation where a widget is added in template, but the mixin "_WidgetsInTemplateMixin" is not present.
This case could be easily detected and if so, should prevent the costly "beforeFillContent" to be executed.
In my specific case, i fixed this by applying a monkey patch:
var oldFillContentMethod = _WidgetsInTemplateMixin.prototype._beforeFillContent; _WidgetsInTemplateMixin.prototype._beforeFillContent = function() { /* start overrule */ if (this.templateString && this.templateString.indexOf("data-dojo-type") > -1) { // just call the "core" method oldFillContentMethod.apply(this, arguments); } /* end overrule */ };
I think this should be added to the "_WidgetsInTemplateMixin", or do i miss something here?
Change History (10)
comment:1 Changed 5 years ago by
Owner: | changed from bill to paulrutter |
---|---|
Status: | new → pending |
comment:2 Changed 5 years ago by
Resolution: | → invalid |
---|---|
Status: | pending → closed |
Because we get so many tickets, we often need to return them to the initial reporter for more information. If that person does not reply within 14 days, the ticket will automatically be closed, and that has happened in this case. If you still are interested in pursuing this issue, feel free to add a comment with the requested information and we will be happy to reopen the ticket if it is still valid. Thanks!
comment:3 Changed 5 years ago by
Sorry, i didn't see (or get) a notification on this issue, so i will respond now.
"widgetsInTemplate: false" would work yes, but why should the developer have to worry about setting this property (which will often be forgotten), when it can also be checked on the fly (making the property superfluous)?
comment:4 Changed 5 years ago by
Resolution: | invalid |
---|---|
Status: | closed → reopened |
I agree, checking on the fly is better.
comment:5 Changed 5 years ago by
Milestone: | tbd → 1.11 |
---|---|
Owner: | changed from paulrutter to bill |
Status: | reopened → assigned |
I tried out your suggested change. Unfortunately it's unreliable. The problem is that the data-dojo-type=...
markup can be hidden inside a substitution variable, rather than appearing directly in this.templateString
. In fact, that happens with ConfirmDialog?, which has the markup of
<div> ... ${!actionBarTemplate} </div>
So the only reliable check is:
/dojoType|data-dojo-type/i.test(this.domNode.innerHTML)
I guess that's still worth it.
comment:6 Changed 5 years ago by
Summary: | Add check to _WidgetsInTemplateMixin -> _beforeFillContent → remove widgetsInTemplate property and instead detect automatically |
---|
comment:7 Changed 5 years ago by
Ah, indeed, i missed that completely. I will use your fix into my monkeypatch also.
comment:8 Changed 5 years ago by
Yup, I missed it too, if it weren't for the ConfirmDialog? regression test I wouldn't have thought of it either.
comment:9 Changed 5 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Hmm, it might be a good safeguard, but can't you just set
widgetsInTemplate: false
in your widget?