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

Error only exists at first BEMHTML.apply #113

Closed sbmaxx closed 8 years ago

sbmaxx commented 8 years ago

How to reproduce:

git clone https://github.yandex-team.ru/sbmaxx/bemhtml-bug.git
cd bemhtml-bug
npm i
npm test

On first run i have: [TypeError: Cannot set property 'onmousedown' of undefined] Second — all is fine.

Templates from islands 4.0.1, "compiled" with enb-bemxjst 4.0.0

sbmaxx commented 8 years ago

@veged @arikon

tadatuta commented 8 years ago

@sbmaxx are you sure it's a bug?

seems like you've got following situation:

block('b1').attrs()(function() {
    var prevAttrs = applyNext(); // undefined as it's just a first template for b1
    prevAttrs.someKey = 'blah'; // can't set property 'someKey' of undefined
});

so all you need is just to add fallback for fiji/images/blocks-deskpad/snippet/_type/snippet_type_series.bemhtml.js like this:

/* ... */
elem('link')(
        attrs()(function() {
            var attrs = applyNext() || {}; // here it is
            this.ctx.counter && (attrs.onmousedown = this.ctx.counter);
            return attrs;
        })
/* ... */
sbmaxx commented 8 years ago

@tadatuta i know that template are ugly and incorrect. The problem is "why it reproduces only once or twice"

tadatuta commented 8 years ago

It's because script failed at https://github.yandex-team.ru/sbmaxx/bemhtml-bug/blob/master/bemhtml.js#L1278 and as so these lines were never reached so on next BEMHTML.apply() call the template is considered already visited.

Still I'm not sure if it's a bug or a feature.

sbmaxx commented 8 years ago

I think it should fail always. Consider how to debug errors if they disappear after refresh? Inspired by migration to islands 4.0.1 ;)

tadatuta commented 8 years ago

adding try catch around out = template.body.call(context); will make it happen but it can be expensive in terms of performance so let's wait for @indutny to take a look.

sbmaxx commented 8 years ago

We can almost level performance with extracted try/catch:

Match.prototype.tryCatch = function(fn, ctx) {
    try {
        return fn.call(ctx);
    } catch(e) {
        return 'SOME_ERROR';
    }
};

Results:

sbmaxx@sbmaxx-mba ~/Develop/bem-xjst/bench $ node run --compare
render:basic:next x 251,794 ops/sec ±1.02% (86 runs sampled)
render:basic:prev x 259,770 ops/sec ±0.49% (87 runs sampled)
render:islands:next x 471 ops/sec ±1.69% (85 runs sampled)
render:islands:prev x 506 ops/sec ±1.26% (85 runs sampled)
render:showcase:next x 983 ops/sec ±1.06% (86 runs sampled)
render:showcase:prev x 972 ops/sec ±2.46% (83 runs sampled)
sbmaxx@sbmaxx-mba ~/Develop/bem-xjst/bench $ node run --compare
render:basic:next x 261,806 ops/sec ±0.38% (87 runs sampled)
render:basic:prev x 255,736 ops/sec ±1.10% (86 runs sampled)
render:islands:next x 489 ops/sec ±1.39% (86 runs sampled)
render:islands:prev x 507 ops/sec ±1.25% (85 runs sampled)
render:showcase:next x 982 ops/sec ±1.14% (85 runs sampled)
render:showcase:prev x 1,037 ops/sec ±2.47% (83 runs sampled)
sbmaxx@sbmaxx-mba ~/Develop/bem-xjst/bench $

Maybe @indutny have better idea :)

sbmaxx commented 8 years ago

Also, i'm not sure that benchmark is very accurate. It gives difference results comparing the same version of bem-xjst

qfox commented 8 years ago

Done via 1ab21c35236b861bae8ffd0d79c3ef1713982d69 and can be closed.