bem / bem-xjst

bem-xjst (eXtensible JavaScript Templates): declarative template engine for the browser and server
https://bem.github.io/bem-xjst
Other
116 stars 48 forks source link

this does not contain special fields like block, elem, mods, elemMods #63

Closed arikon closed 9 years ago

arikon commented 9 years ago

Compiled in production mode template is

function __$b37(__$ctx, __$ref) {
   console.log("this.block", $$block);
   console.log("this.elem", $$elem);
   console.log("this.mods", $$mods);
   console.log("this.elemMods", $$elemMods);
   console.log("this.ctx", __$ctx.ctx);
   return (__$ctx.ctx.items || []).map(function expand__$81(item) {
       console.log("this.block", this.block);
       console.log("this.elem", this.elem);
       console.log("this.mods", this.mods);
       console.log("this.elemMods", this.elemMods);
       console.log("this.ctx", this.ctx);
       if (this.isSimple(item)) {
           item = {
               text: item
           };
       }
       if (item.items) {
           item.elem = "group";
           item.content || (item.content = item.items.map(expand__$81, this));
       } else {
           item.elem = "item";
           if ([ "check", "radio", "radiocheck" ].indexOf(this.mods.type) !== -1) {
               item.elemMods = this.extend({
                   type: "option",
                   checked: this.ctx.val.indexOf(item.val) !== -1 ? "yes" : undefined
               }, item.elemMods);
           } else {
               if (this.mods.type === "navigation") {
                   item.elemMods = this.extend({
                       type: "link"
                   }, item.elemMods);
               }
           }
       }
       return item;
   }, __$ctx);
}

The output is:

this.block menu
this.elem undefined
this.mods { type: 'check', theme: 'normal', size: 's' }
this.elemMods {}
this.ctx { block: 'menu',
 mods: { type: 'check', theme: 'normal', size: 's' },
 items: [ 'Item 1', 'Item 2', 'Item 3' ],
 val: [] }

this.block undefined
this.elem undefined
this.mods undefined
this.elemMods undefined
this.ctx { block: 'menu',
 mods: { type: 'check', theme: 'normal', size: 's' },
 items: [ 'Item 1', 'Item 2', 'Item 3' ],
 val: [] }

The problem is in the second block of console.log()'s. __$ctx, that is passed as a context to the map() does not contain special fields like block, elem, mods, elemMods.

arikon commented 9 years ago

Full template source

block('menu')(
    mode('tabindex')(undefined),

    def()(function() {
        var ctx = this.ctx,
            tabindex = this.ctx.tabindex || apply('tabindex');

        ctx.val = ctx.hasOwnProperty('val') ? [].concat(ctx.val) : [];
        if (['radio', 'radiocheck'].indexOf(this.mods.type) !== -1) {
            ctx.val = ctx.val.slice(0, 1)
        }

        return applyNext({_tabindex: tabindex});
    }),

    js()(function() {
        return {_tabindex: this._tabindex}
    }),

    attrs()(function() {
        return {
            tabindex: (this.mods.disabled ? undefined : this._tabindex),
            'aria-disabled': (this.mods.disabled && 'true')
        }
    }),

    match(function() { return !this.ctx.content; }).content()(function() {
        console.log('this.block', this.block);
        console.log('this.elem', this.elem);
        console.log('this.mods', this.mods);
        console.log('this.elemMods', this.elemMods);
        console.log('this.ctx', this.ctx);
        return (this.ctx.items || []).map(function expand(item) {
            console.log('this.block', this.block);
            console.log('this.elem', this.elem);
            console.log('this.mods', this.mods);
            console.log('this.elemMods', this.elemMods);
            console.log('this.ctx', this.ctx);
            if (this.isSimple(item)) {
                item = {text: item};
            }

            if (item.items) {
                item.elem = 'group';
                item.content || (item.content = item.items.map(expand, this));
            } else {
                item.elem = 'item';

                if (['check', 'radio', 'radiocheck'].indexOf(this.mods.type) !== -1) {
                    item.elemMods = this.extend({
                        type: 'option',
                        checked: (this.ctx.val.indexOf(item.val) !== -1) ? 'yes' : undefined
                    }, item.elemMods);
                } else {
                    if (this.mods.type === 'navigation') {
                        item.elemMods = this.extend({
                            type: 'link'
                        }, item.elemMods);
                    }
                }
            }

            return item;
        }, this);
    })
);
vkz commented 9 years ago

@indutny appears that Compiler.prototype.replaceContext = function replaceContext(ast) { in bem-xjst doesn't go deep enough to replace this.block and friends with globals $$block etc. Should it though?

vkz commented 9 years ago

or should we also have those fields available in __$ctx?

arikon commented 9 years ago

I think it would be better to extend __$ctx with block and friends. Because not all this.block expressions in the nested code blocks would mean .block from the template context.

vkz commented 9 years ago

@arikon is right, no telling what nested functions could be referencing with this.block

vkz commented 9 years ago

@indutny so my guess would be that the culprits are rollOutApply and friends that generate:

                                $$elem = _$4elem__$71;
                                var __$l7__$78 = $$mods;
                                $$mods = _$4mods__$72;
                                var __$l8__$79 = $$elemMods;
                                $$elemMods = _$4elemMods__$73;
                                var __$l9__$80 = $$mode;
                                $$mode = "mix";
                                __$r__$75 = applyc(__$ctx, __$ref);
                                $$block = __$l5__$76;
                                $$elem = __$l6__$77;
                                $$mods = __$l7__$78;
                                $$elemMods = __$l8__$79;
                                $$mode = __$l9__$80;

the result is that the most recent context is in the globals, but not in the __$ctx passed to __$r__$75 = applyc(__$ctx, __$ref);.

arikon commented 9 years ago

It also should be possible to do

var this_ = this;
console.log(this_.block);

For now it outputs undefined.

vkz commented 9 years ago

actually it's the combination of rollOutLocal in xjst which generates the above code with __$ctx.block etc in the assignments, so the context is presevrved, but then replaceContext in bem-xjst compiler renames the __$ctx.block etc so that __$ctx passed to applyc doesn't reflect proper context.

indutny commented 9 years ago

var this_ = this; won't work, we have fixed most of the places where it was used. It is not a legal way to fetch variables from the context, I'm afraid.

indutny commented 9 years ago

I have just pushed out xjst@1.5.1 with vkz's fixes, going to figure out what should be done here to make it work.

indutny commented 9 years ago

And xjst@1.5.2 with all of the fixes.

indutny commented 9 years ago

Anyway, for now I propose to store var block = this.block; outside of the map's function to make it work. Does it make sense? Is there any other problem that should be fixed?

arikon commented 9 years ago

It is not a legal way to fetch variables from the context, I'm afraid.

@indutny It is not obvious for the js developer, I guess =(

arikon commented 9 years ago

Updated xjst to 1.5.3, all the tests are passing except the ones that relate on this issue.

indutny commented 9 years ago

Yeah, this is unfortunate semantics that we are bound to: this.x = 1 does not work and _this.x does not work properly either. Working on the "proper" fix for the map thing, may make _this work too.

vkz commented 9 years ago

First step that could help @indutny have a look pls https://github.com/vkz/xjst/commit/636be9b7e28b646656613c98acd207105b19cf75

indutny commented 9 years ago

@vkz I'm afraid this will come with a performance penalty. Though, the fix that I am working on atm is a bit similar to yours in some sense.

vkz commented 9 years ago

@indutny no sweat :) just wanted to know if I was thinking in the right direction. More than happy for you to save the day hehehe

indutny commented 9 years ago

Actually, I felt a de ja vu here :) Looks like I already did it for bem-bl. May I ask you to give a try to https://github.com/bem/bem-xjst/pull/65 ?

indutny commented 9 years ago

I think it is fixed now.