Skip to content
This repository

Global and deep stack context functions have wrong 'this' pointer. #117

Closed
wants to merge 6 commits into from

5 participants

Will Shaver Don't Add Me To Your Organization a.k.a The Travis Bot Veena Basavaraj jimmyhchan Steven
Will Shaver

Related to: #96 (20 days previous to this post)

The fix in this pull request always uses the 'current context'

elem = elem.apply(context.current(), [this, context, null, {auto: auto, filters: filters}]);

The testing framework doesn't allow for more complicated contexts to be easily defined as it expects a simple single-stack context. Consider the following more complicated context and global stack:

var context = dust.makeBase({
 name: '1Bob',
 getName1: function(){ return this.name;}});
var instance = context.push({
 name: '2Joe',
 getName2: function(){ return this.name;}});
instance = instance.push({
 name: '3Tim',
 getName3: function(){ return this.name;}});

With the dust template:

{getName1}
{getName2}
{getName3}

The current implementation returns:
3Bob3Bob3Bob

Please indicate where and how to put a test with a more complicated context and I'll be happy to add one.

To solve this issue, we need to bind the function at compile time. Using underscore (just to get it done quickly) the code looks like this:

Context.prototype.get = function(key) {
  var ctx = this.stack, value;
  while(ctx) {
    if (ctx.isObject) {
      value = ctx.head[key];
      if(typeof value === 'function'){
        value = _.bind(value, ctx.head);
      }
      if (value !== undefined) {
        return value;
      }
    }
    ctx = ctx.tail;
  }
  if(this.global){
    value = this.global[key];
    if(typeof value === 'function'){
      value = _.bind(value, this.global);
    }
    return value;
  }
  return undefined;
};

This fixes the global / deep stack issue and passes the unit tests defined in pull request 96. I've implemented a minimal 'bind' inside of the .git function to avoid the use of underscore. Please tell me where to add tests for this issue. I've added some manual tests in a site I'm building, but would like to add tests in the linkedin project.

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged a69332c into d0affe6).

Veena Basavaraj
Collaborator
vybs commented

in the process of reviewing. thanks @wshaver

Will Shaver

https://github.com/wshaver/speck This is some requirejs + dustjs + backbone glue I wrote. I turns the backbone view into the global context for the dustjs snippet, so that the snippets can call members of the view. (With this patch anyway.)

Veena Basavaraj
Collaborator

@wshaver Sorry for the delay, but overall + 1 on the idea and also thanks for bringing this up.

Are you still looking for pointers to add tests?

Adding a complex context is no different than here
https://github.com/linkedin/dustjs/blob/master/test/jasmine-test/spec/grammarTests.js

Will Shaver

Is this the file I should add tests to for this issue?

Veena Basavaraj vybs commented on the diff
lib/dust.js
@@ -133,18 +133,32 @@ Context.wrap = function(context) {
133 133 };
134 134
135 135 Context.prototype.get = function(key) {
  136 + var bind = function(context, func){
  137 + return function(){
8
Veena Basavaraj Collaborator
vybs added a note

can we move this out of get?

Where would you like it?

Veena Basavaraj Collaborator
vybs added a note

one more q:
would not the following need the same check?

Context.prototype.getBlock = function(key, chk, ctx) {
if (typeof key === "function") {
key = key(chk, ctx).data;
chk.data = "";
}

dynamic partials

if (typeof elem === "function") {
return this.capture(elem, context, function(name, chunk) {
dust.load(name, chunk, context).end();
});

It might, I'll try and add a test for both.

Can you provide an example of a partial that includes a function? I'll work on this some more tomorrow - nap time.

Veena Basavaraj Collaborator
vybs added a note

sure :) thanks for your help.

I was hinting at the dynamic partials, {">partial{name}":overridecontext/} and the context itself been a function. I have not personally used this ( but the code supports it )

Will Shaver
wshaver added a note

So where did you want the bind function?

Will Shaver
wshaver added a note

Also I haven't ever used dynamic partials, and therefore my ability to write a test for them is minimal. As long as this code isn't breaking any existing tests, to do we need more tests? Can't someone using d-partials submit a bug if they're not working?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Veena Basavaraj
Collaborator

you can also create a new one if need be, since its been a plan to refactor the grammar and dust-core tests out.

Will Shaver

How do I create a global context inside grammarTests.js? That's what I was not sure about before.

Veena Basavaraj
Collaborator

Ah, to simulate global context,

may be extend the to have a globalContext, just like we have context in the specs

Will Shaver wshaver Merge remote-tracking branch 'wshaver/master'
Conflicts:
	lib/dust.js
	test/jasmine-test/spec/grammarTests.js
8b32ad5
Will Shaver

Actually we can do it by changing the order of the includes in the specRunner. Then you can have a test like:

  {
    name:     "test that deep stack functions have the correct scope",
    source:   "{getName1}{getName2}{getName3}",
    context:  (function(){
                var context = dust.makeBase({
                  name: '1Bob',
                  getName1: function(){ return this.name;}
                });
                var instance = context.push({
                  name: '2Joe',
                  getName2: function(){ return this.name;}
                });
                instance = instance.push({
                  name: '3Tim',
                  getName3: function(){ return this.name;}
                });
                return instance;
              }()),
    expected: "1Bob2Joe3Tim",
    message: "Should allow for deep stack functions"
  }
Veena Basavaraj vybs commented on the diff
lib/dust.js
((11 lines not shown))
142 146 while(ctx) {
143 147 if (ctx.isObject) {
144 148 value = ctx.head[key];
145   - if (!(value === undefined)) {
  149 + if(typeof value === 'function'){
2
Veena Basavaraj Collaborator
vybs added a note

another note: in order to distinguish between the dust compiler created functions and the lambda's that can exist as part of the context , we have
the elem.isFunction check

see the tap method fyi

Will Shaver
wshaver added a note

I'm not quite sure what you're saying here. The value here is likely outside of dust as it can be part of the 'context'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Veena Basavaraj
Collaborator

@wshaver do let me know when this is good to be reviewed.

Veena Basavaraj
Collaborator

ping? @wshaver

Will Shaver

Hey there, sorry I've been busy with other projects. I've dumped my fork and rebuilt it. Should I issue a separate pull request?

Veena Basavaraj
Collaborator

Welcome back ! sure with the changes we talked about:)

Will Shaver

I asked a couple of other comments above clarifying the changes you want.

Veena Basavaraj vybs closed this
Veena Basavaraj
Collaborator
vybs commented

closed, since it is unclear if this is a priority to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
28 lib/dust.js
@@ -137,18 +137,32 @@ Context.wrap = function(context) {
137 137 };
138 138
139 139 Context.prototype.get = function(key) {
  140 + var bind = function(context, func){
  141 + return function(){
  142 + return func.apply(context, arguments);
  143 + };
  144 + };
140 145 var ctx = this.stack, value;
141   -
142 146 while(ctx) {
143 147 if (ctx.isObject) {
144 148 value = ctx.head[key];
145   - if (!(value === undefined)) {
  149 + if(typeof value === 'function'){
  150 + value = bind(ctx.head, value);
  151 + }
  152 + if (value !== undefined) {
146 153 return value;
147 154 }
148 155 }
149 156 ctx = ctx.tail;
150 157 }
151   - return this.global ? this.global[key] : undefined;
  158 + if(this.global){
  159 + value = this.global[key];
  160 + if(typeof value === 'function'){
  161 + value = bind(this.global, value);
  162 + }
  163 + return value;
  164 + }
  165 + return undefined;
152 166 };
153 167
154 168 Context.prototype.getPath = function(cur, down) {
@@ -362,10 +376,8 @@ Chunk.prototype.render = function(body, context) {
362 376
363 377 Chunk.prototype.reference = function(elem, context, auto, filters) {
364 378 if (typeof elem === "function") {
365   - elem.isFunction = true;
366   - // Changed the function calling to use apply with the current context to make sure
367   - // that "this" is wat we expect it to be inside the function
368   - elem = elem.apply(context.current(), [this, context, null, {auto: auto, filters: filters}]);
  379 + elem.isReference = true;
  380 + elem = elem(this, context, null, {auto: auto, filters: filters});
369 381 if (elem instanceof Chunk) {
370 382 return elem;
371 383 }
@@ -379,7 +391,7 @@ Chunk.prototype.reference = function(elem, context, auto, filters) {
379 391
380 392 Chunk.prototype.section = function(elem, context, bodies, params) {
381 393 if (typeof elem === "function") {
382   - elem = elem.apply(context.current(), [this, context, bodies, params]);
  394 + elem = elem(this, context, bodies, params);
383 395 if (elem instanceof Chunk) {
384 396 return elem;
385 397 }
5 test/jasmine-test/client/specRunner.html
@@ -10,12 +10,13 @@
10 10 <script type="text/javascript" src="lib/jasmine-1.1.0/jasmine.js"></script>
11 11 <script type="text/javascript" src="lib/jasmine-1.1.0/jasmine-html.js"></script>
12 12
  13 + <!-- include source files here... -->
  14 + <script src="../../../dist/dust-full-1.0.0.js" type="text/javascript" charset="utf-8"></script>
  15 +
13 16 <!-- include spec files here... -->
14 17 <script src="../spec/grammarTests.js" type="text/javascript" charset="utf-8"></script>
15 18 <script src="../spec/renderTestSpec.js" type="text/javascript" charset="utf-8"></script>
16 19
17   - <!-- include source files here... -->
18   - <script src="../../../dist/dust-full-1.0.0.js" type="text/javascript" charset="utf-8"></script>
19 20
20 21 <script type="text/javascript">
21 22 (function() {
21 test/jasmine-test/spec/grammarTests.js
@@ -675,6 +675,27 @@ var grammarTests = [
675 675 context: {"xhr": false},
676 676 expected: "tag not found!",
677 677 message: "should test child template with dash"
  678 + },
  679 + {
  680 + name: "test that deep stack functions have the correct scope",
  681 + source: "{getName1}{getName2}{getName3}",
  682 + context: (function(){
  683 + var context = dust.makeBase({
  684 + name: '1Bob',
  685 + getName1: function(){ return this.name;}
  686 + });
  687 + var instance = context.push({
  688 + name: '2Joe',
  689 + getName2: function(){ return this.name;}
  690 + });
  691 + instance = instance.push({
  692 + name: '3Tim',
  693 + getName3: function(){ return this.name;}
  694 + });
  695 + return instance;
  696 + }()),
  697 + expected: "1Bob2Joe3Tim",
  698 + message: "Should allow for deep stack functions"
678 699 }
679 700 ];
680 701

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.