Opened 8 years ago

Closed 8 years ago

#13779 closed enhancement (wontfix)

parser scope / way to avoid globals

Reported by: bill Owned by: bill
Priority: high Milestone: 1.8
Component: Parser Version: 1.7.0b1
Keywords: Cc: Adam Peller, Colin Snover, ben hockey, cjolif
Blocked By: Blocking:

Description (last modified by bill)

The AMD conversion tries to eliminate all globals, but the parser still needs them for two cases:

1) data-dojo-props

data-dojo-props, and also individual widget attributes like onClick, often reference globals:

data-dojo-props="store: myStore, onClick: myOnClick"

This could be handled by passing a scope/map to the parser.

3) data-dojo-id

Conversely to the above case, the parser also creates globals:

data-dojo-id="myStore"

There should be a way to control where the myStore variable is created.

Since often #3 and #2 are related, they should probably be controlled by the same scope map. Example of relation:

<div data-dojo-type="dojo.store.Memory" data-dojo-id="myStore"/>
<select data-dojo-type="dijit.form.ComboBox data-dojo-props="store: myStore">

NOTE: this description was modified from it's original to remove work already done as part of #13778.

Attachments (4)

parser.html.patch (2.6 KB) - added by Kitson Kelly 8 years ago.
Initial Cut of Patch
parser.js.patch (5.6 KB) - added by Kitson Kelly 8 years ago.
First cut of patch
test_contextParser.html (2.0 KB) - added by Kitson Kelly 8 years ago.
Something that tests points 1,2,3,4
parser.js.2.patch (5.7 KB) - added by Kitson Kelly 8 years ago.
Fixes args passing and dojoType object getting

Download all attachments as: .zip

Change History (37)

comment:1 Changed 8 years ago by Colin Snover

For #1, I’m not entirely clear why we need users to manually require modules. There is no reason that I can think of that the parser ought not be able to do something like:

var foundElements = findWidgetElements();

require(foundElements.map(function(element){
  return element.getAttribute("data-dojo-type");
}), function(){
    var args = arguments;
    foundElements.forEach(function(element, i){
        var clazz = args[i];
        new clazz({ … }, element);
    });
});

…but I don’t know all the things people expect to be able to do with it, so please enlighten me if I am forgetting something important. (In re: builds, I would expect the build system to be able to parse the HTML files for data-dojo-types too?)

For #3, what about allowing a user to specify a module to attach them to, with some reasonable default? Like this:

<div data-dojo-id="foo"></div>

require([ "dojo/parser" ], function(parser){
    parser.widgets.foo; // the div
});

// or

require([ "dojo/parser", "my/app" ], function(parser, app){
    parser.parse({ namespace: app });
    app.foo; // the div
});

Dunno about #2.

Last edited 8 years ago by Colin Snover (previous) (diff)

comment:2 in reply to:  1 Changed 8 years ago by bill

Replying to csnover:

For #1, I’m not entirely clear why we need users to manually require modules.

We don't need to, see #13778.

…but I don’t know all the things people expect to be able to do with it, so please enlighten me if I am forgetting something important. (In re: builds, I would expect the build system to be able to parse the HTML files for data-dojo-types too?)

Hadn't thought about builds; that's an extra complication. But see the #13778 for the other issues.

For #3, what about allowing a user to specify a module to attach them to, with some reasonable default? Like this:

require([ "dojo/parser", "my/app" ], function(parser, app){

parser.parse({ namespace: app }); app.foo; the div

}); }}}

It looks like (from the parser's point of view) app is just a plain javascript hash, aka scope/map, like I'm suggesting to use in this ticket. Did I miss something?

comment:3 Changed 8 years ago by ben hockey

Cc: ben hockey added

comment:4 Changed 8 years ago by cjolif

Cc: cjolif added

comment:5 Changed 8 years ago by Kitson Kelly

Bill, would an approach for this (assuming this is coupled with #13778 that we introduce two args keywords of globals and resources?

resources would be a hash of variables that would be used during parsing and for attribute parsing would take precedence over any globals referenced data-dojo-props (it could also be checked for data-dojo-type too).

globals would default to true for backwards compatibility, but when set to false it would take any data-dojo-id and add it to the hash upon instantiation and return the hash at the end.

So something like this would be possible:

var resources = new Array();
resources['myOnClick'] = function(evt) {
  console.log("I was clicked');
};

var parserReturn = parser.parse({
  globals: false,
  resources: resources
});

var myStore = parserReturn['myStore'];

I can't see any major issues with this, so I am start prepping a patch based on this, but will take any feedback before I finish it (and the changes to the tests).

comment:6 Changed 8 years ago by Kitson Kelly

Hmmm, just realised the instantiate() and parser() return an array of parsed and instantiated objects. So, if I change this, I think I would break some things.

I did a grep of the source and found the following:

  • ./demos/mobileGallery/src/base.js calls startup() on each widget returned.
  • ./dijit/_WidgetsInTemplateMixin.js uses the results to startup its widgets
  • ./dojo/html.js sets its parseResults based on the return
  • ./dojox/dtl/* heavily depends on the return results

So the only way I can see to avoid this is to put the results of the resources into a variable of the parser object, which I will proceed with resources. So that either the resources the following would be possible:

parser.resources['myOnClick'] = function(evt) {
  console.log("I was clicked');
};

parser.parse({
  globals: false
});

var myStore = parser.resources['myStore'];

I will still support the passing of resources in the kwArgs for parse().

I will also update the livedocs to make it clear that return hash is expected behaviour.

comment:7 Changed 8 years ago by bill

That sounds pretty good, but rather than setting a "resources" attribute on the parser class, you should pass it in as a parameter to the parse() function:

var ctx = {
   myOnClick: function(evt){
       console.log("I was clicked');
   }
};

parser.parse({
  context: ctx
});

var myStore = ctx.myStore;

You don't need a globals parameter either... just make context, when unspecified, default to kernel.global.

BTW there are a few strange things about your initial example:

var resources = new Array();
resources['myOnClick'] = function(evt) {
  console.log("I was clicked');
};

When you want to create an array the canonical way is

resource = [];

but in this case you don't really want an array, but rather a hash, so it should be:

resources = {};

Also, to set an element in a hash it's better to do:

resources.myOnClick = ...;

rather than

resources['myOnClick'] = ...;

and even better than that is to set it as part of the declaration of the resources variable:

var resources = {
   myOnClick: function(evt){
       console.log("I was clicked');
   }
};

comment:8 Changed 8 years ago by Kitson Kelly

As far as the hashes are concerned, I have been playing with it and realised most of what you suggested Bill, as I started to look at the coding style in the parser.

I will change it to context and drop the globals.

I still think having access to any parsed objects with data-dojo-id set should be available after the parse() finishes (at least it is useful to me for having access to any declarative datastores for example), so I am assuming I will set the context attribute of the parser object.

comment:9 Changed 8 years ago by Kitson Kelly

I think I have run into a problem with data-dojo-props. Currently the code has the following to convert the attribute value into an object to be passed on construction:

extra = djson.fromJson.call(args.propsThis, "{" + extra + "}");

But this causes extra to only be evaluated in the global scope. In order for it to consider the local context, I have had to do the following:

with (this.context) {
  extra = eval("({" + extra + "})");
}

But of course this doesn't handle the args.propThis and doesn't put the layer of "abstraction" that dojo.fromJson() offered. For now, I am considering these mutually exclusive (since args.propsThis should only be used by dijit._WidgetsInTemplateMixin and shouldn't be passing additional context) and will do the following:

if (args.propsThis) {
  extra = djson.fromJson.call(args.propsThis, "{" + extra + "}");
} else {
  with (this.context) {
    extra = eval("({" + extra + "})");
  }
}

comment:10 Changed 8 years ago by bill

Note that the current call to fromJson(), although politically correct, is not accurate... data-dojo-props is not JSON because it contains global variables and this references ex: data-dojo-props="onClick: myOnClick, foo: this.x".

So probably something like this will work and makes sense:

function evalProps(context, props){
   with(context){
       return eval("({" + props + "})");
   }
}
extra = evalProps.call(args.propsThis, this.context, extra);

(Haven't tried it though.)

comment:11 Changed 8 years ago by Kitson Kelly

Yup, that seems to work.

Ok, so I have points 1, 2 and 3 resolved. There is one more point that I think we need to address (of which I am still working on trying to figure out) in that declarative scripting should take advantage of the passed context.

4) <script type="dojo/*">

Currently, the parser creates declarative script in a way it only has access to items created in a global context, therefore the scripts do not have access to baseless modules. For example if we do the following:

require(["dojo/parser", "dojo/on"], function(parser,on) {
  parser.parse();
}
<div data-dojo-type="...">
  <script type="dojo/on" data-dojo-event="click" data-dojo-args="e">
    console.log("I was clicked");
    on(dijit.byId("myObject"), "click", function(event) {
      //Something happens
    });
  </script>
</div>

The declarative script won't have access to the "on" function, although the parser will properly attempt to attach the script to the instantiated object.

There are challenges though with the current process of how we dynamically generate the scripts, and while I think I can apply the this.context to the script (through .apply although it isn't a complete thought in my head yet), we may have to substantially restructure that part of the code. We might have to take the approach of rolling the data-dojo-with into the .apply with the this.context. I am doing some more thinking on it, but as usual, any additional thoughts are much appreciated.

comment:12 Changed 8 years ago by bill

Good point on #4, glad you are thinking through these issues. I guess, like you said, it would have to be handled through the context object again, although I'm dreading having to build a giant context object every time I want to call the parser. I guess that's part of the price of going global-less.

Changed 8 years ago by Kitson Kelly

Attachment: parser.html.patch added

Initial Cut of Patch

comment:13 Changed 8 years ago by Kitson Kelly

Ok, here is my first cut of the patch, which is causing 9 errors and 2 failures in the DOH tests, so I am going to dig into those a little bit more and see why it is failing, but I figured I would throw up the patch for anyone to look at and comment on. I have an app that attempts all points 1-4 and they work, but I need to figure out how I introduced the regressions.

As far as point 4, we could consider a "parser context" and a "script context" as two separate things, since there is unlikely going to be overlap in the two contexts, except for point 3, in which the parser could add those to both contexts.

Changed 8 years ago by Kitson Kelly

Attachment: parser.js.patch added

First cut of patch

comment:14 Changed 8 years ago by Kitson Kelly

Ooops, ignore the parser.html.patch. That was an accident and I don't have permissions to delete attachments on Trac.

comment:15 Changed 8 years ago by ben hockey

can you provide the test that you have so that if i try to tweak your patch i can know if i break something?

there's a couple of things that i'd like to play with:

  • why does the parser build up this.context? i would think the context should only relate to one run of the parser. maybe i'm missing something.
  • why do we need an extra with and eval in _functionFromScript? could you have just tweaked preamble and still used new Function( ... )?
  • maybe we could work out context once (including whether or not its the global context) and then uniformly just use that context rather than branching at a number of places. for example:
ctor = _ctorMap[type] || (_ctorMap[type] = (this.context[type] || dlang.getObject(type)))

could become:

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

which is even more powerful because now you can also access "foo.Bar" in your provided context rather than just "foo".

those are just a few to get started... at least if you can provide your test i can make sure that i don't break the intention of the code.

comment:16 Changed 8 years ago by ben hockey

btw, its a little disheartening that we're moving even further from what would pass in es5 strict mode but i guess that's the price of "magic". however, i want to be sure that we don't break any code that previously used strict mode and worked. there is one small issue with dojo.declare that breaks anything using strict mode but i'd like to make sure we don't introduce any other issues so that if that one problem can be fixed, our consumers can write code in strict mode even though we don't.

i haven't noticed anything yet that i'm certain would break but just wanted to flag the concern to make sure its taken into consideration if possible.

Changed 8 years ago by Kitson Kelly

Attachment: test_contextParser.html added

Something that tests points 1,2,3,4

comment:17 Changed 8 years ago by Kitson Kelly

Ok, I have made a test case, but I realised that point 3 isn't working right anymore in the first version of the patch. I will look into, but please "hack" away...

  • I think I "built up" the context because originally I thought someone might have supplied it before invoking .parse() but then I agreed with Bill we wouldn't do that, but had forgotten I had done it.
  • I thought about tweaking the preamble but I wasn't sure how that would actually work in practice. I first started coding that way but then tried to figure out the context would actually be implemented as a string. If there is an easy way, I am willing to learn.
  • Couldn't figure out how to pass a "global" context as a variable, but doing the following should work:
ctor = _ctorMap[type] || (_ctorMap[type] = 
  (dlang.getObject(type, false, context) || dlang.getObject(type, false)))

I don't like the eval() and the heavy use of with() but I can't figure any other ways for magic. Especially because with() being not strict es5 strict, but I can't figure any other way. We also are "stuck" with eval for the JSON conversion because we would really break things because data-dojo-props isn't strict JSON and it is a pain in the "ass" declaratively.

comment:18 Changed 8 years ago by Kitson Kelly

Ok, the main regression I seem to introduce is that everytime I try to parse() in tests/parser.html I throw an error at the if (!ctor) that says Error: Could not load class 'tests.parser.Button', but it isn't the changes that I made to the ctor =.

What is odd is that console.log(_ctorMap) demonstrates that there is a 'tests.parser.Button' but console.log(_ctorMap[type]) and console.log(_ctorMap["tests.parser.Button"] both come back as undefined.

comment:19 Changed 8 years ago by Kitson Kelly

Ok, I found it. I thought I was fixing a bug, but it has some undesired effects. It has to do with rootNode and args. The documentation for the parser (including some of the inline examples) indicates that you should be able to do the following:

parser.parser({
  context: {}
});

Only passing the args and no rootNode, but when I was doing that in my examples, I wasn't getting anything populated in the args variable, so I changed this:

var root;
if(!args && rootNode && rootNode.rootNode){
	args = rootNode;
	root = args.rootNode;
}else{
	root = rootNode;
}

To this:

var root;
if(!args && rootNode && rootNode.rootNode){
	args = rootNode;
	root = args.rootNode;
}else if(!args && rootNode) {
	args = rootNode;
}else{
	root = rootNode;
}

Which sets args when rootNode isn't passed, but that seems to cause a problem somewhere in the parser when a node is passed in rootNode but the args is not passed. So I need to find an efficient way to check if rootNode is a DOM node (and there doesn't seem to be anything within Dojo that does that on first glance). I will work on that later, but that seems to be the cause of 100% of the regressions I introduced.

comment:20 Changed 8 years ago by ben hockey

a typical way to sniff if an object is a DOM node is to check for the presence of the nodeType property - this is why there's no helper in dojo to do this.

also, i took a look at the code regarding attaching the context to the parser (this.context). i'm fairly sure that we don't want to attach a single context to the parser that is shared between every run of the parser. here's why, in one run of the parser if i pass in a foo in the context that "overrides" a global foo, in the next run of the parser if i intend to pick up the global foo, i'll actually get the context foo - not what i was expecting. another case, if on both runs of the parser i have a data-dojo-id of foo but pass in different contexts then the 2nd run will override the foo from the first run of the parser and any code from the first run that tries to access parser.context.foo will now get the 2nd foo - again, not the expected behavior.

this means that the test case should look more like this (notice that i'm keeping a reference to the context that i pass in and that's how i get back whatever got attached to the context):

require([
        "dojo/parser",
        "dojo/on",
        "dijit/form/Button",
        "dijit/layout/BorderContainer",
        "dijit/layout/ContentPane",
        "dijit/layout/TabContainer"
],
function (parser,on,Button) {

        var myOnClick = function(evt) {
                console.log("I was clicked!");
        }

        var myObject = {
                        test: "text"
        }

        var context = {
                myOnClick: myOnClick,
                myObject: myObject,
                myClass: Button
        };

        parser.parse({
                context: context
        });

        context.button3.set("label","I Changed!");
});

in trying this approach, here's what i came up against. let's take the structure from your test case:

	<div data-dojo-type="dijit.layout.BorderContainer" id="main" data-dojo-props="gutters:true" style="width:100%; height:100%;">
		<div data-dojo-type="dijit.layout.ContentPane" id="header" data-dojo-props="splitter:false, region:'top'" style="height: 35px;">Header...</div>
		<div data-dojo-type="dijit.layout.TabContainer" id="tabcont" data-dojo-props="region:'center', tabStrip:true">
			<div data-dojo-type="dijit.layout.ContentPane" id="content" data-dojo-props="title:'Content', selected:true">
				<div data-dojo-type="dijit.form.Button" id="button1" data-dojo-props="onClick: myOnClick">Button 1</div>
				<div data-dojo-type="dijit.form.Button" id="button2">Button 2
					<script type="dojo/on" data-dojo-event="click" data-dojo-args="e">
						console.log(this.label + " was clicked!");
						console.log(myObject);
					</script>
				</div>
				<div data-dojo-type="myClass" data-dojo-id="button3" id="button3">Button 3</div>
			</div>
		</div>
	</div>

the parser will successfully use the original context to parse the BorderContainer? and its descendants in each branch as far as the first descendant with a stopParser flag - ie in this case that's everything up to and including the first ContentPane? in each branch; header and content. the problem comes when those ContentPanes? try to parse their children - they no longer have the original context. currently it works because there is the shared context for all runs of the parser but as i mentioned earlier, we should not have a shared context for all runs of the parser.

the question is... should we be sharing that original context with the ContentPanes? and how would we do that? this is starting to get kind of messy now.

comment:21 Changed 8 years ago by Kitson Kelly

First, thanks for the pointer.

Second, I am unconvinced the single context per invocation is a problem, because the global context is a single context, and so namespace "clashes" are just as likely there as any other context. The problem with not sharing the context is that we don't have an easy way of dealing with subsequent calls to the parser (like when the ContextPane? invokes the parser). Currently, it does persist through that entire parse.

If users needed a different context, they could invoke the parse and setting the rootNode and context on each invocation. I guess the one thing that I might do is always ensure the context is cleared on every invokation of parse(), even if no context is passed in the args.

Also, as I stated above, we might want to consider providing a optional separate context just for declarative scripting, so if the scripting context is supplied, one context would be used for instantiation and one for scripting.

Changed 8 years ago by Kitson Kelly

Attachment: parser.js.2.patch added

Fixes args passing and dojoType object getting

comment:22 Changed 8 years ago by Kitson Kelly

Ok, parser.js.2.patch does the following:

  • Properly fixes the passing of args without a rootNode and all DOH tests are passing again
  • Properly fixes data-dojo-id scoping
  • Properly retrieves constructors using getObject()
  • The parser no longer "builds" the context, but instead replaces the context if one is supplied

So I think the questions that are open are:

  • Should we persist the context per parser until it is overwritten?
  • Is there a better way to do the _functionFromScript that uses new Function() instead of eval()?
  • Does this patch give due consideration to ES5 Strict compliance and only deviate because it has to?

I will start working on the DOH test cases. In my mind we need at least the following:

  • One positive case for points 1, 2, 3, 4 in both global and passed context.
  • A test case for "complex" objects (e.g. "test.myObject")
  • A test for point 3 where it referenced by additional parsing and referenced after the parse.
  • A test case where rootNode and args are both passed and where args only are passed
  • Parser "chaining" test cases (related to scope issue)

comment:23 in reply to:  21 Changed 8 years ago by ben hockey

Replying to kitsonk:

Second, I am unconvinced the single context per invocation is a problem, because the global context is a single context, and so namespace "clashes" are just as likely there as any other context. The problem with not sharing the context is that we don't have an easy way of dealing with subsequent calls to the parser (like when the ContextPane?? invokes the parser). Currently, it does persist through that entire parse.

a single context per invocation is what i'm suggesting. i have a problem with having a single context for all invocations. if its ok for context to be like the global scope then this feature adds no value compared to exposing everything as globals before calling parse. part of what should be achieved by this feature is to improve on the single global scope by allowing the use of different context objects to avoid these clashes.

you're right that context currently persists through an entire parse but it also persists through ALL parses - that's the problem. keep in mind that in your test file, one call to parser.parse actually invokes the parser 3 times for that test - once at the top level and once for each ContentPane?. we would probably like to use the same context for those 3 calls but then we don't want it to hang around after that.

If users needed a different context, they could invoke the parse and setting the rootNode and context on each invocation. I guess the one thing that I might do is always ensure the context is cleared on every invokation of parse(), even if no context is passed in the args.

this won't fix the problem. if you clear the context every time parse is invoked it is the same as not keeping it via this.context - the parsing from the ContentPanes? would be a new invocation of parse and would clear the context.

Also, as I stated above, we might want to consider providing a optional separate context just for declarative scripting, so if the scripting context is supplied, one context would be used for instantiation and one for scripting.

it seems that there already exists a separate scripting context - it's achieved via the with attribute of a script. it seems undocumented and and untested and i never knew it existed until i was looking at the code yesterday. as with the current state of things in the parser, the object referred to by with needs to be a global.

this is obviously a feature for those that prefer declarative code. however, by the time you've written the code in your "declarative" app to build up all these contexts and call the parser i don't see why its that much more difficult to have written a custom templated widget with attach points and other various pieces to make that widget be the context you're trying to build. its a great deal more powerful without needing to jump through hoops to build up a dummy environment/context.

comments on the updated patch to come...

comment:24 Changed 8 years ago by bill

The problem of sending the context to ContentPanes (because of their stopParser: true flag) is indeed a sticky issue. I see why @kitsonk wants to persist the context past the end of the parse() call, although admittedly that has it's own set of problems.

Keep in mind (to make matters worse), in 2.0 I want ContentPanes inside of StackContainers to defer their parse until the pane is selected... which makes the ContentPane parse() very asynchronous from the original parse() call.

comment:25 in reply to:  24 Changed 8 years ago by Kitson Kelly

Yes, I tried to clear the context per invocation, but basically you get no value of passing the context if the parser calls are chained, since I can't think of a way to easily pass/not pass context on chained parser calls except to persist it in the parser (and it is also the only way to allow access to the results of the parse for point 3, since we can't change the return value of the parser without breaking lots of stuff).

I will do some thinking on this to get my mind around it.

Right now each parser has a single context (global), what the patch does is create a second "private" context per parser which behaves in much the same way the single global one does.

There is the big 2.0 discussion, because #13778 isn't fully solvable without a significant restructure of how the parser operates, which we are also saying is a requirement for 2.0, but how far are we going to move in 1.8 with the parser? I suspect we would solve this totally differently in 2.0.

In my opinion this "half-way" solution that allows me use baseless code declaratively without manually scoping things and fixes the global scoping of data-dojo-id/jsId, which actually strangely solves #10799 as well, which has bugged me for almost 2 years.

comment:26 Changed 8 years ago by ben hockey

i just don't buy that this solution does anything that couldn't be done by manually exposing everything as globals.

require(['dojo/parser', 'my/Widget'], function (parser, MyWidget) {
  parser.parse({
    context: {
      MyWidget: MyWidget
    }
  });
});

compared to:

require(['dojo/parser', 'my/Widget'], function (parser, MyWidget) {
  window.MyWidget = MyWidget;
  parser.parse();
});

in either case, somewhere you're going to need a small block of programmatic code to help bootstrap things and the difference between these 2 pieces of code is negligible except that the 2nd snippet doesn't need a change to the parser right now. what is it about the 1st block that makes it more palatable than the 2nd?

if the context is not somehow sandboxed from unassociated runs of the parser then its no better than the current state.

comment:27 Changed 8 years ago by Kitson Kelly

Ok, I put my thinking cap on a bit and thought about another whole way to tackle this beast, getting at what might have been part of your point Ben (or maybe not, but it inspired me anyways).

What we could do is extend the declarative syntax to fully encompass some of the problems faced with the move to AMD/baseless, so we would introduce "control nodes" that don't specifically refer objects to be instantiated. Something like:

<div data-dojo-type="require" data-dojo-props="module:'dijit/form/Button'"></div>

<div data-dojo-type="require" data-dojo-context="a" 
  data-dojo-props="module:'dojo/on',name:'on'"></div>
<div data-dojo-type="dijit.form.Button">
  <script type="dojo/on" data-dojo-event="click" data-dojo-context="a">
    on(dijit.byId("..."),"click", function(e) {...});
  </script>
</div>

<script type="dojo/function" data-dojo-context="b" data-dojo-args="a,b,c" 
  data-dojo-props="name:'myFunction'">
    console.log("myFunction");
</script>
<div data-dojo-type="dijit.form.Button" data-dojo-context="b" 
  data-dojo-props="onClick:myFunction"></div>

The syntax could do with a bit of tweaking, but it removes the "bootstrap" things from programmatic and moves it fully into declarative. parseOnLoad could be enabled. From a declarative religious point, I would much prefer that. Now that I typed it out, I actually like that a whole lot more. Are we up for extending the parser to declarative support require and context? I think I could deliver all 4 points through something like that, but it might mean a larger restructure of parser for 1.8, since it runs into the issues with the async nature of require but it could fully solve #13778 as well as potentially place us in a situation of meeting some of the 2.0 objectives.

I had been thinking about this anyways, because not fully solving #13778 was bothering me. The parser sort of does this now:

  • Walks the DOM and collects nodes
  • forEach each node
    • Determines its type
    • Looks up its Constructor
    • Analyse Prototype
    • Instantiates the Object
  • Passes the objects back as return

What we would have to do is restructure it so that we loop through a couple times, something like:

  • Walks the DOM and collects nodes
  • forEach each node determining type or control node
  • require
  • forEach each node
    • Lookup Constructor
    • Lookup Prototype
    • Analyse Prototype
    • Instantiate
  • Return Objects

What the only "disadvantage" I see is there would be a level of time spent working on nodes which may not get anything done with them once the prototype is analysed.

(Sorry for being long winded)

comment:28 Changed 8 years ago by bill

@neonstalwart - If you are suggesting to forget about the context and just use globals, it's a valid argument. If you have another idea about how to sandbox the context and still have it work across ContentPane etc., please list it.

@kitsonk - I see what you are proposing. It's nice that it's all declarative, although this is all getting very complicated, and I'm not sure if it's worth it.

comment:29 in reply to:  28 Changed 8 years ago by Kitson Kelly

Replying to bill:

@kitsonk - I see what you are proposing. It's nice that it's all declarative, although this is all getting very complicated, and I'm not sure if it's worth it.

While it might not actually go anywhere, I am enjoying the process and learning things, so for me it is worth it. From a religious point of view, there are mainly 5 objectives in my mind, and if I can achieve them then I think we could put it to a vote. The objectives would be:

  • Restructure the parser to make it potentially easier to modify/extend behaviour
  • Don't decrease the performance of the parser
  • Bring as much feature parity into the parser as possible with programmatic
  • Solve points 1-4
  • Solve #13778

Was there anything else major on the deck for 1.8 for the parser? (Outside of #13778)

comment:30 Changed 8 years ago by ben hockey

bill I'm thinking this is feeling like we're trying to shoehorn in something that doesn't fit. I'm saying to stick with globals, I don't have a suggestion for how to sandbox the context but I am convinced that if we're going to do it then that's how it should work.

comment:31 Changed 8 years ago by ben hockey

Using module ids rather than globals for data-dojo-type or some other attribute is a different story though. I believe that is worthwhile and should be achievable.

comment:32 in reply to:  31 Changed 8 years ago by Kitson Kelly

Replying to neonstalwart:

Using module ids rather than globals for data-dojo-type or some other attribute is a different story though. I believe that is worthwhile and should be achievable.

I will add my patch to #13778 then, because it is very straight forward but has a couple of problems in my mind.

comment:33 Changed 8 years ago by bill

Description: modified (diff)
Resolution: wontfix
Status: newclosed

OK, I'm going to close this until/unless we come up with a plan for globals that we all like. The ticket has gotten really long too.

Note: See TracTickets for help on using tickets.