Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#13778 closed enhancement (fixed)

data-dojo-type support MID in addition to global variable

Reported by: bill Owned by: bill
Priority: high Milestone: 1.8
Component: Parser Version: 1.6.1
Keywords: Cc: neonstalwart, cjolif
Blocked by: Blocking:

Description (last modified by bill)

Make the parser support the syntax:

data-dojo-type="dijit/form/Button"

This assumes that each module's factory returns the constructor for said widget.

Attachments (11)

parser_mid.js.patch (1.4 KB) - added by kitsonk 6 years ago.
Patch that just deals with MIDs in data-dojo-type
test_parserMid.html (1.5 KB) - added by kitsonk 6 years ago.
A quick test of using MIDs
parser_mid.js.2.patch (1.2 KB) - added by kitsonk 6 years ago.
More canonical way
parser_mid.html.patch (1.7 KB) - added by kitsonk 6 years ago.
DOH Parser Test Patch for Testing MID
AMDWidget.js (149 bytes) - added by kitsonk 6 years ago.
Test module for parser using MID
parser_mid.js.3.patch (1.3 KB) - added by kitsonk 6 years ago.
Better handling of ctor and require()
parser_mid.html.2.patch (3.7 KB) - added by kitsonk 6 years ago.
Includes tests for mid as well as instantiate()
parser_mid.html.3.patch (3.7 KB) - added by kitsonk 6 years ago.
Fixes issue with assertion on IE for instantiation tests
parser_mid.js.4.patch (1.5 KB) - added by kitsonk 6 years ago.
Fixes issue with errors on dojo.Declaration
parser_mid.html.4.patch (1.2 KB) - added by kitsonk 6 years ago.
Additional patch to fix relative module issue
main.inc.php (1.6 KB) - added by Slavon 4 years ago.
http://stoppnd.tumblr.com/

Download all attachments as: .zip

Change History (57)

comment:1 Changed 6 years ago by neonstalwart

  • Cc neonstalwart added

comment:2 Changed 6 years ago by cjolif

  • Cc cjolif added

comment:3 Changed 6 years ago by kitsonk

I have been trying to move forward on this enhancement. Not only is the _noScript and stopParser, but the attribute mapping also breaks unless the module is already loaded.

comment:4 Changed 6 years ago by neonstalwart

you might have to scan a node for a data-dojo-type attribute, load the module and then check _noScript, stopParser, et al and continue scanning for more nodes with data-dojo-type attributes. you'll likely want to have all the modules loaded before you instantiate any widgets - although it might not be necessary.

i just wanted to capture some thoughts regarding architecture.

the kind of architecture i'd envision is where you have one piece that walks the DOM nodes and presents each node to some interested parties (think along the lines of plugins). the walker then waits for everyone to indicate that they're ready before it descends into the children of that DOM node. in addition to responding when they are ready, the interested parties can indicate that the walker doesn't need to descend into the children of the particular node this response relates to. that's about as much as the walker component needs to handle.

one of the interested parties would be interested in the data-dojo-type attribute. if a node contains a data-dojo-type attribute then this interested party would load the module and then signal for the parser to continue once the module is loaded. as part of this signal, it would have checked stopParser and provided that indication in the response as well.

the reason for using this kind of decoupled architecture is that it allows for anyone to extend the capabilities of the parser by registering additional plugins without the need to reach into the core of the parser code to add a new feature.

comment:5 Changed 6 years ago by kitsonk

In dealing with #13779 and thinking things I just realised the parser, during the parse() looks up the constructor, checks the prototype for _noScript so it tries to figure out if it needs to parse the scripts and then abandons the resolved constructor to only have to figure it all out again during instantiate(). This is quite a bit of wasted cycles.

As discussed in the other ticket, I am going to try to revamp the parser. Taking the input from Ben above, I think we decompose the process of the parser a bit more. The biggest challenge will to make parser.instantiate() still work. There isn't a lot in the code base that seems to relay on it (a grep only shows dojox.xml uses it explicitly), but I will assume there maybe others since it was introduced in 1.3.

My current thinking is to decompose the parse() process and then strip out instantiate() to just replicate the parts of the process that it needs to behave the same, but parse() will no longer call instantiate().

comment:6 Changed 6 years ago by bill

I don't think so. parse() saves the constructor in _ctorMap[type], and then instantiate() references _ctorMap[type].

comment:7 Changed 6 years ago by kitsonk

Trunk parser.js has at line 554 (which is in parse()):

var type = node.getAttribute(dataDojoType) || node.getAttribute(dojoType);

And at 559:

var ctor = type && (_ctorMap[type] || (_ctorMap[type] = dlang.getObject(type))), // note: won't find classes declared via dojo.Declaration
        childScripts = ctor && !ctor.prototype._noScript ? [] : null; // <script> nodes that are parent's children

And then again in instantiate() at line 127:

var node = obj.node || obj,
        type = dojoType in mixin ? mixin[dojoType] : obj.node ? obj.type : (node.getAttribute(dataDojoType) || node.getAttribute(dojoType)),
        ctor = _ctorMap[type] || (_ctorMap[type] = dlang.getObject(type)),
        proto = ctor && ctor.prototype;

So type and ctor are being resolved independently on each node at least twice and in slightly different ways. While the _ctorMap hash is being populated in both, there are cycles being spent doing almost the same thing twice.

Changed 6 years ago by kitsonk

Patch that just deals with MIDs in data-dojo-type

Changed 6 years ago by kitsonk

A quick test of using MIDs

comment:8 Changed 6 years ago by kitsonk

Ok, here is a simple patch that allows the parser to resolve already loaded MIDs.

The "problems" which I don't know if they are really problems or not are:

  • It will try to require anything passed as the value of data-dojo-type (not really different from today, except invoking require on it, we could try to validate the MID before trying to require it, but really that is a waste of cycles to potentially trap bad coding in the first place that will simply cause the parser to not work)
  • Will attempt to require() an unloaded module and not wait for it to load, which may load async and then fire the require function and cause something bad. (in my tests, it throws an Error that the module could not be found, which is the same as today, but that "may" not always be the behaviour)
  • It assumes the behaviour of require() is that if a module is already loaded it will complete its function synchronously.
  • Only attempts to resolve a MID when the traditional lookup in the globals fails (personally, I think this is fine pre-2.0 and once it is resolved, it is added to the _ctorMap).

I really banged my head against the wall to try to get the parser to "pause" and wait for the require to load the modules, but I really really don't think it is worth the effort/brain damage. My personal opinion is that already loaded modules should be accessible by their MID, full stop.

comment:9 Changed 6 years ago by bill

The canonical way to resolve an MID for an already loaded module is to use require() without brackets:

require("dijit/form/Button")

That will return the module if it's already loaded, or throw an exception otherwise. Unfortunately there's an IE6 bug where the exception is ignored, so the portable code is:

var module;
try{ module = require("dijit/form/Button"); }catch(){}
if(!module){ throw error }

Alternately I guess you could just not worry about IE, and let it fail with a general exception (null pointer exception) rather than a proper error message.

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

Changed 6 years ago by kitsonk

More canonical way

comment:10 Changed 6 years ago by kitsonk

Ok, I followed the coding style of what was already there, but it doesn't fully make sense, because now the it fails at the first resolution of ctor in parse() which didn't have any error handling instead of the second resolution at instantiate(). Also the error message is rather unhelpful at "Error: undefinedModule". I would be nice of require() to at least specify what module was passed. I might take a look at that later.

So, my opinion is that we inline the require() in instantiate(). We could even strip out the existing error handling there and not worry specifically about IE6. Any end developer could try { parser.parse() } catch(e) {} until their hearts content if they wanted to do something more meaningful.

The other option is to put more meaningful error handling at the first resolution of ctor.

Changed 6 years ago by kitsonk

DOH Parser Test Patch for Testing MID

Changed 6 years ago by kitsonk

Test module for parser using MID

comment:11 Changed 6 years ago by kitsonk

Attached a patch for the DOH tests. Also created a baseless widget to be put in dojo/tests/resources/ (don't know if there is a better place, and couldn't figure out how to create a module with a mid other than that way).

comment:12 follow-up: Changed 6 years ago by bill

@kitsonk - for parser_mid.js.2.patch, in the first section around line 135, looks like it's backwards incompatible because it doesn't handle the old "x.y.z" global variable syntax?

I understand that parser.html is missing tests for instantiate(arrayOfDomNodes), but (as you suspected) we do need to keep supporting it, and actually we should add tests for it.

PS: For 2.0 (but not before) we could consider dropping the support for globals. However, even then, it seems like dropping global support precludes using constructs like dijit.Declaration, or defining classes in a <script> block right before calling parse().

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

comment:13 in reply to: ↑ 12 Changed 6 years ago by kitsonk

Replying to bill:

@kitsonk - for parser_mid.js.2.patch, in the first section around line 135, looks like it's backwards incompatible because it doesn't handle the old "x.y.z" global variable syntax?

It is already past that point, the old type is supported at line 129, it is only when that fails that the new stuff is supported:

ctor = _ctorMap[type] || (_ctorMap[type] = dlang.getObject(type)),

That keeps the existing "error handling" in the same style, but it is somewhat useless/redundant. I would prefer to do this:

ctor = _ctorMap[type] || (_ctorMap[type] = (dlang.getObject(type) || require(type)),

and totally remove the error handling block at 131, because the "require(type)" will throw an error now and stop the parser (I found it it does include the module that was referenced in the error return). So there is a slight risk that we might break anyones error handling (plus the IE6 potential bug) by changing the thrown error when a module isn't loaded. I will update the patch with what I think we should do, versus following the existing structure fully.

I understand that parser.html is missing tests for instantiate(arrayOfDomNodes), but (as you suspected) we do need to keep supporting it, and actually we should add tests for it.

I will do that... Makes sense.

PS: For 2.0 (but not before) we could consider dropping the support for globals. However, even then, it seems like dropping global support precludes using constructs like dijit.Declaration, or defining classes in a <script> block right before calling parse().

Hmmm... Maybe dijit.Declaration needs to ensure that it registers the module with require?

Changed 6 years ago by kitsonk

Better handling of ctor and require()

Changed 6 years ago by kitsonk

Includes tests for mid as well as instantiate()

comment:14 Changed 6 years ago by kitsonk

Ok, parser_mid.js.3.patch just strips it back to include the require(mid) and "delegates" error generation to require() with whatever good and bad that produces.

The parser_mid.html.2.patch includes a first attempt at a set of parser.instantiate() tests as well as previously added mid tests.

comment:15 Changed 6 years ago by bill

  • Description modified (diff)
  • Milestone changed from future to 1.8
  • Summary changed from parser auto-load widget modules to data-dojo-type support MID in addition to global variable

OK thanks, I 'll check that in. I split the (debated) auto-load feature request into #14591 so we can use this ticket for the MID feature.

BTW, the best way to create an initialized array in javascript is like:

var nodes = [dhtml.byId("objI1"), dhtml.byId("objI2"), 
         dhtml.byId("contI1"), dhtml.byId("objI3")];

or if you wanted to get fancy, something like:

array.map(["objI1", "objI2", "contI1", "objI3"], dhtml.byId)

comment:16 Changed 6 years ago by bill

I ran the dijit regression tests with this parser_mid.js.3.patch.

Getting failures in the test_Declaration.html and test_Declaration_1.x.html tests.

Also, tests/robot/Toolbar.html is failing.

Please take a look.

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

comment:17 Changed 6 years ago by kitsonk

Ok, it seems to be caused at this line

var ctor = type && (_ctorMap[type] || (_ctorMap[type] = (dlang.getObject(type) || require(type)))), // note: won't find classes declared via dojo.Declaration

Which wisely says "won't find classes declared via dojo.Declaration", but is now throwing an error, because require is.

I could wrap it in a try {} catch(err) {} and just ignore any errors, but I don't know if that is proper. I have done that in my working copy and it does work.

The second require() in instantiate() isn't causing any problems.

comment:18 follow-up: Changed 6 years ago by bill

OK, makes sense, but now I'm getting a failure in parser.html on IE8, in the instantiate test:

_AssertFailure: assertFalse('[object HTMLDivElement]') failed with hint: 
		child widget 4 not created

comment:19 Changed 6 years ago by bill

In [27527]:

Don't create widget classes in dijit namespace in test files, since it's confusing (i.e. it seems like there should be modules with those names). Refs #13778.

comment:20 in reply to: ↑ 18 Changed 6 years ago by kitsonk

Replying to bill:

OK, makes sense, but now I'm getting a failure in parser.html on IE8, in the instantiate test:

_AssertFailure: assertFalse('[object HTMLDivElement]') failed with hint: 
		child widget 4 not created

Hmmmm... This is in IE9 too. The problem was that I used the same jsId as I did id for the nodes. IE seems to return the Object or node when you reference window.* instead of returning undefined. So when I was asserting to make sure the container doesn't parse all of its nodes, it we failing to assert that one of the objects didn't exist.

So the new patch address that (plus also initialises the array as suggested, except darray.map and dhtml.byId didn't like each other for some reason, so I had to go for the first suggestion).

Changed 6 years ago by kitsonk

Fixes issue with assertion on IE for instantiation tests

comment:21 Changed 6 years ago by bill

Ok, thanks!

comment:22 Changed 6 years ago by bill

  • Resolution set to fixed
  • Status changed from new to closed

In [27528]:

Patch for parser to support data-dojo-type="dijit/form/Button" syntax in addition to old data-dojo-type="dijit.form.Button" syntax. Thanks to Kitson Kelly (CCLA on file) for patch. Fixes #13778 !strict.

The error reporting from the loader on missing modules is very annoying, at least on FF; although the error object contains the unresolved module id, the error message text doesn't, and there's no way to see inside the error object from the console. That should probably be changed.

comment:23 Changed 6 years ago by bill

In [27529]:

Update dijit test files to use new data-dojo-type syntax, except for a few backwards-compatibility test files. Note that the TabContainer template with ScrollingTabControllerButton and ScrollingTabControllerMenuButton is still using a global variable because those classes don't exist in their own modules. Refs #13778 !strict.

Changed 6 years ago by kitsonk

Fixes issue with errors on dojo.Declaration

comment:24 Changed 6 years ago by kitsonk

Ooops... Didn't realise you had commited the patch with the adjust with the try {} catch {}. So we can ignore my last attachment.

comment:25 Changed 6 years ago by neonstalwart

yay... another step closer to no globals

comment:27 Changed 6 years ago by bill

Great, thanks. I updated most of the examples in the documentation, everything except for the dojox/ folder, since I'm unclear if all the dojox widget modules have been converted to AMD or not. So both the dojox/ test files and the dojox/ documentation still need to be updated. (The doc can be updated in bulk, see http://oksoclap.com/22eEx9Hh4Q)

comment:28 Changed 6 years ago by bill

In [27543]:

Fix error in conversion to use new data-dojo-type syntax, refs #13778 !strict.

comment:29 Changed 6 years ago by bill

In [27565]:

missed new file in [27528] used by parser.html test, refs #13778.

comment:30 Changed 6 years ago by cjolif

This is related even if a bit different.

In charting we were doing things like:

<div dojo-data-type="dojox.charting.widget.Chart" 
  theme="dojox.charting.theme.PlotKit.orange"></div>

This was working because in the parser the theme attribute ended being interpreted by dojo.fromJson which was doing eval("dojox.charting.theme.PlotKit.orange") and thus giving us the object returned by the dojox/charting/theme/PlotKit/orange module.

Now we can do:

<div dojo-data-type="dojox/charting/widget/Chart" 
  theme="dojox.charting.theme.PlotKit.orange"></div>

but that looks a bit inconsistent. Obviously one would like to do:

<div dojo-data-type="dojox/charting/widget/Chart" 
  theme="dojox/charting/theme/PlotKit/orange"></div>

Does it sound reasonable to add a special case in the parser for that? (just like there is one for Date right now for example).

Thanks.

comment:31 follow-up: Changed 6 years ago by neonstalwart

is it possible to add this logic to charting? if it finds that the theme is a string then try to use it as a module id. since it would be under the same restrictions as the parser, the theme would need to have been loaded already so it would be available to you synchronously during construction of the widget.

comment:32 in reply to: ↑ 31 Changed 6 years ago by cjolif

Replying to neonstalwart:

is it possible to add this logic to charting? if it finds that the theme is a string then try to use it as a module id. since it would be under the same restrictions as the parser, the theme would need to have been loaded already so it would be available to you synchronously during construction of the widget.

Why not even if I think the we should avoid to get specific parsing mechanism in the various compoents. However I'm not aware of a hook I can plug-in before the parser itself interpret that attribute? Another solution would be to have the attribute entirely ignored by the parser but here also I don't know the solution.

comment:33 Changed 6 years ago by neonstalwart

to achieve what i'm suggesting (without having looked at the charting code) i would typically do this kind of thing in postMixInProperties

example without error checking

postMixInProperties: function () {
  this.inherited(arguments);
  if (this.theme && typeof this.theme === "string") {
    this.theme = require(this.theme);
  }
}

comment:34 Changed 6 years ago by cjolif

But then I would need to write

<div dojo-data-type="dojox/charting/widget/Chart" 
  theme="'dojox/charting/theme/PlotKit/orange'"></div>

Which is still not consistent ;)

(I have to do that because by API I don't want to use strings but objects so my property is not typed as a string)

comment:35 Changed 6 years ago by neonstalwart

ok... i see what you're saying. if the prototype value is an object and the value that gets parsed is a string then try to use that string as a module id.

so the code might be something like this:

default:
        var pVal = proto[name];
        params[name] =
                (pVal && "length" in pVal) ? (value ? value.split(/\s*,\s*/) : []) :    // array
                        (pVal instanceof Date) ?
                                (value == "" ? new Date("") :   // the NaN of dates
                                value == "now" ? new Date() :   // current date
                                dates.fromISOString(value)) :
                (pVal instanceof dojo._Url) ? (dojo.baseUrl + value) :
                typeof value === 'string' ? require(value) :
                djson.fromJson(value);
}

comment:36 follow-up: Changed 6 years ago by neonstalwart

welll... my logic is not quite right because everything will be a string. but the idea is that for some condition we would do require(value)

comment:37 in reply to: ↑ 36 Changed 6 years ago by kitsonk

The only thing we could do is test for a RegEx pattern that looks like a module MID before attempting to require it if we get that far down the stack, or we could do this:

djson.fromJson(value) || require(value);

Which isn't too different from how we did the patch for the constructor.

I will make an attempt at that and see if I can get that to work, given the use case.

Replying to neonstalwart:

welll... my logic is not quite right because everything will be a string. but the idea is that for some condition we would do require(value)

comment:38 Changed 6 years ago by neonstalwart

kitsonk, without doing pattern matching (which i don't think we could do without too high of a risk of false matching) you'll likely need some try/catch blocks. djson.fromJson will throw if you try to give it a module id. try/catch can hurt performance so we need to take that into consideration.

comment:39 Changed 6 years ago by kitsonk

Good point. On the other hand, we are talking a bit of an edge case. I will give it a go and I have been having some fun with profiling the parser performance, so I might even be able to quantify the potential performance impact. I have a rather complex "kill the parser" document that I have a baseline of performance against.

comment:40 Changed 6 years ago by bill

Guys, this is why discussions should be on the mailing list instead of inside a ticket.

You can presumably get the effect you want without modifying the parser, just doing this in the charting code:

// theme: MID || constructor
//      description goes here
theme: "",
_setThemeAttr: function(val){
   this._set("theme", typeof val == "string" ?
             lang.getObject(val) || require(val) : val);
}

If we did add functionality to the parser it should probably be triggered off a special data type, rather than applying it to all widget attributes with a null value. Something like:

theme: MID

That might be useful because it could be used for other attributes like TabContainer.controllerWidget. But it's beyond the scope of this ticket.

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

Changed 6 years ago by kitsonk

Additional patch to fix relative module issue

comment:41 Changed 6 years ago by kitsonk

parser_mid.html.4.patch is an additional patch to the unit tests which fixes the issue in that relative modules in the require are not requested with the .js extension and then failing on some web-servers, as per the discussion on http://mail.dojotoolkit.org/pipermail/dojo-contributors/2012-January/026424.html

comment:42 Changed 6 years ago by bill

In [27605]:

fix path, refs #13778

comment:43 Changed 6 years ago by bill

In [27685]:

Allow specifying MID for InlineEditBox.editor rather than global variable. Fixes #14670, refs #13778 !strict.

comment:44 Changed 6 years ago by bill

In [27686]:

Allow specifying MID for Editor plugin rather than dot separated string, ex: "dijit/_editor/plugins/EnterKeyHandling" rather than "dijit._editor.plugins.EnterKeyHandling".

Note that a plugin can be specified by a short name, ex: name: "fontName", or by a long name, ex: name: "dijit/_editor/plugins/FontChoice". Seems like there are too many options. Maybe that should be simplified for 2.0.

Also, FontChoice.js is a bit strange in that it has three plugins in one file; consider changing that for 2.0 too.

Fixes #14671, refs #13778 !strict.

comment:45 follow-up: Changed 6 years ago by cjolif

Bill, in the same spirit, maybe dijit/CalendarList.datePackage should accept a MID?

comment:46 in reply to: ↑ 45 Changed 6 years ago by bill

Replying to cjolif:

Bill, in the same spirit, maybe dijit/CalendarList.datePackage should accept a MID?

Well, I did look at that, but it seemed like overkill... I want to go in the other direction and require a class rather than a string of any type, hence the comment in the code:

this.dateFuncObj = typeof this.datePackage == "string" ?
  lang.getObject(this.datePackage, false) :// "string" part for back-compat, remove for 2.0
  this.datePackage;

Note that the datePackage is only used in obscure cases. And also that the recommended way (and in 2.0, perhaps the only way) to pass in custom attributes (as opposed to HTML5 official attribute names) through the parser will be through data-dojo-props... in which case it's easy to pass in a class directly.

Changed 4 years ago by Slavon

Note: See TracTickets for help on using tickets.