-
Notifications
You must be signed in to change notification settings - Fork 48
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
bem-xjst: clean cached flags for wrap() and extend() (fix for #495) #496
base: master
Are you sure you want to change the base?
Conversation
Travis alive. |
@tadatuta ping |
I'm not sure that shallow copy is a good fix :( For example such case is still broken: const bemhtml = require('bem-xjst').bemhtml;
const tmpl = bemhtml.compile(function() {
block('b3').wrap()(node => ({
block: 'b2',
content: node.ctx
}));
});
const bemjson = { block: 'b1', content: { block: 'b3' } };
const firstTime = tmpl.apply(bemjson);
const secondTime = tmpl.apply(bemjson);
console.log('firstTime:', firstTime);
console.log('secondTime:', secondTime); |
@tadatuta OK, that was a quick attempt. The bug is deeply related to https://github.com/bem/bem-xjst/blob/master/lib/bemxjst/match.js#L46-L47 and it’s flag prevent infinity loop. Right now I have no idea how to fix it… :( Do you have any suggestions? |
@zxqfox we discussed possible fix with @miripiruni few days ago. |
95373cc
to
5b5aa73
Compare
5b5aa73
to
10550d5
Compare
2% slower then previous solution with shallow copy? |
measurement error ~1%. It’s normal measurement error on my macbook with turned off network. After optimisation commit results looks better:
I tend to think of this as an insignificant speed change. |
f3eb923
to
285637c
Compare
Fixes #495
Changes proposed in this pull request
On every
templates.apply(bemjson)
call we can use copy ofbemjson
, not thebemjson
by link.Checklist
@tadatuta if it looks like production decision for you, I can run benchmarks.