bem / bem-xjst

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

wrap() mutates bemjson which can lead to error for second run for same input data #495

Open tadatuta opened 6 years ago

tadatuta commented 6 years ago

Input code or something about issue background

const bemhtml = require('bem-xjst').bemhtml;
const tmpl = bemhtml.compile(function() {
    block('b1').wrap()(node => ({
        block: 'b2',
        content: node.ctx
    }));
});

const bemjson = { block: 'b1' };

const firstTime = tmpl.apply(bemjson);
const secondTime = tmpl.apply(bemjson);

console.log('firstTime:', firstTime);
console.log('secondTime:', secondTime);

Important!

  1. bemjson should be passed by reference
  2. the same instance of compiled template should be used

Expected Behavior

firstTime: <div class="b2"><div class="b1"></div></div>
secondTime: <div class="b2"><div class="b1"></div></div>

Actual Behavior

firstTime: <div class="b2"><div class="b1"></div></div>
secondTime: <div class="b1"></div>

Your Environment

Any version of bem-xjst with wrap() support.

// cc @vithar

miripiruni commented 6 years ago

The same issue with extend():

$ cat extend.js
const bemhtml = require('./').bemhtml;
const tmpl = bemhtml.compile(function() {
    block('b1').extend()({ 'ctx.content': 42 });
});

const bemjson = { block: 'b1' };

const firstTime = tmpl.apply(bemjson);
console.log('firstTime:', firstTime);
console.log(bemjson);

const secondTime = tmpl.apply(bemjson);
console.log('secondTime:', secondTime);
console.log(bemjson);

$ node extend.js
firstTime: <div class="b1">42</div>
{ block: 'b1', content: undefined }
secondTime: <div class="b1"></div>
{ block: 'b1', content: undefined }
miripiruni commented 6 years ago

@tadatuta I see only one solution: copy bemjson to a new object in first line of bemhtml.apply(). Is it OK?

qfox commented 6 years ago

Prob if we can't fix this, we can deny this.

Actual Behavior

firstTime: <div class="b2"><div class="b1"></div></div>
secondTime: Exception with xjst can't go in to the same river twice

Probably we can generate some id for the xjst passes on the apply call and use it for _wrap flag? Cuz cloning objects is a thing we don't want to do because of speed.

Or create an ctx with a Set of used wrappers and extends for a node (object) in a bemjson tree (invert the structure) to prevent mutations of bemjson at all.

qfox commented 6 years ago

Guess, right now we have the same thing with deeper placed objects:

  it('should work with several apply() calls', function() {
    var bemjson = [ { tag: 'span', content: { block: 'b1' } } ];
    var expected = '<span><div class="b1">42</div></span>';
    var tmpl = fixtures.compile(function() {
      block('b1').extend()({ 'ctx.content': 42 });
    });

    assert.equal(
      tmpl.apply(bemjson),
      expected,
      'first apply() call returns not expected value'
    );

    assert.equal(
      tmpl.apply(bemjson),
      expected,
      'second apply() call returns not expected value'
    );
  });
vithar commented 5 years ago

Any news here?