Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tadatuta opened this issue Jan 16, 2018 · 5 comments

Comments

@tadatuta
Copy link
Member

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
Copy link
Contributor

miripiruni commented Jan 16, 2018

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
Copy link
Contributor

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

@qfox
Copy link
Member

qfox commented Jan 23, 2018

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
Copy link
Member

qfox commented Jan 23, 2018

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
Copy link
Member

vithar commented Nov 15, 2018

Any news here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants