Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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 3 years ago by bill

Owner: changed from bill to paulrutter
Status: newpending

Hmm, it might be a good safeguard, but can't you just set widgetsInTemplate: false in your widget?

comment:2 Changed 3 years ago by trac-o-bot

Resolution: invalid
Status: pendingclosed

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 3 years ago by paulrutter

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 3 years ago by bill

Resolution: invalid
Status: closedreopened

I agree, checking on the fly is better.

comment:5 Changed 3 years ago by bill

Milestone: tbd1.11
Owner: changed from paulrutter to bill
Status: reopenedassigned

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 3 years ago by bill

Summary: Add check to _WidgetsInTemplateMixin -> _beforeFillContentremove widgetsInTemplate property and instead detect automatically

comment:7 Changed 3 years ago by paulrutter

Ah, indeed, i missed that completely. I will use your fix into my monkeypatch also.

comment:8 Changed 3 years ago by bill

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 3 years ago by Bill Keese <bill@…>

Resolution: fixed
Status: assignedclosed

In 4aa5ea0b3c82a0c4accf6ce184dd842861caf08b/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:10 Changed 3 years ago by paulrutter

Great, thanks for your swift response!

Note: See TracTickets for help on using tickets.