Improved mix behaviour #197

miripiruni · 2016-02-15T14:51:21Z

+15 test cases
–1 bug

Fixed (degradation)

1. bemhtml should duplicate block class if mix several block with mods to elem in the same block.

Because block class must have for mix block with mods to block elem.

Example:

({
    block: 'b',
    content: {
        elem: 'e',
        mix: [
            { block: 'b', mods: { m1: 'v1' } },
            { block: 'b', mods: { m2: 'v2' } }
        ]
    }
});

4.3.4 result:

<div class="b"><div class="b__e b b_m1_v1 b b_m2_v2"></div></div>

demo

5.0.0 result:

<div class="b"><div class="b__e b b"></div></div>

demo;

This PR result:

<div class="b"><div class="b__e b b_m1_v1 b b_m2_v2"></div></div>

Improved

2. bemhtml should not duplicate block class if mix is the same block with mods.

({
    block: 'b',
    mix: [
        { block: 'b', mods: { m1: 'v1' } },
        { block: 'b', mods: { m2: 'v2' } }
    ]
});

4.3.4 result:

<div class="b b b_m1_v1 b b_m2_v2"></div>

demo

5.0.0 result:

<div class="b b b_m1_v1 b b_m2_v2"></div>

demo

This PR result:

<div class="b b_m1_v1 b_m2_v2"></div>

3. bemhtml should not duplicate elem class if mix is the same elem.

Weird case, but for completeness why not to check it

({
    block: 'b',
    elem: 'e',
    mix: { elem: 'e' }
});

4.3.4 result:

<div class="b__e b__e"></div>

demo

5.0.0 result:

<div class="b__e b__e"></div>

demo

This PR result:

<div class="b__e"></div>

4. bemhtml should not duplicate elem class if mix is the same block elem.

Weird case, but for completeness why not to check it.

({
    block: 'b',
    elem: 'e',
    mix: { block: 'b', elem: 'e' }
});

4.3.4 result:

<div class="b__e b__e"></div>

demo

5.0.0 result:

<div class="b__e b__e"></div>

demo

This PR result:

<div class="b__e"></div>

5. bemhtml should not duplicate elem class if mix the same elem to elem in block.

Weird case, but for completeness why not to check it.

({
    block: 'b',
    content: {
        elem: 'e',
        mix: { elem: 'e' }
    }
});

4.3.4 result:

<div class="b"><div class="b__e b__e"></div></div>

demo

5.0.0 result:

<div class="b"><div class="b__e b__e"></div></div>

demo

This PR result:

<div class="b"><div class="b__e"></div></div>

6. bemhtml should not duplicate elem class if mix is the same block elem with elemMods.

({
    block: 'b',
    elem: 'e',
    mix: { elem: 'e', elemMods: { modname: 'modval' } }
});

4.3.4 result:

<div class="b__e b__e b__e_modname_modval"></div>

demo

5.0.0 result:

<div class="b__e b__e b__e_modname_modval"></div>

demo

This PR result:

<div class="b__e b__e_modname_modval"></div>

7. bemhtml should not duplicate block elem elemMods class

({
    block: 'b',
    elem: 'e',
    mix: { block: 'b', elem: 'e', elemMods: { modname: 'modval' } }
});

4.3.4 result:

<div class="b__e b__e b__e_modname_modval"></div>

demo

5.0.0 result:

<div class="b__e b__e b__e_modname_modval"></div>

demo

This PR result:

<div class="b__e b__e_modname_modval"></div>

“Who cares” cases (zero cost but enjoyable)

Weird cases, but for completeness why not to check it.

8. bemhtml should duplicate block mods class if mix is the same block with mods.

But who cares? It’s pretty rare case.
To fix this we need to compare each key/value pairs. It’s too expensive.
I believe that developers should not want to do this.

({
    block: 'b',
    mods: { m: 'v' },
    mix: { block: 'b', mods: { m: 'v' } }
});

4.3.4 result:

<div class="b b_m_v b b_m_v"></div>

demo

5.0.0 result:

<div class="b b_m_v b b_m_v"></div>

demo

This PR result:

<div class="b b_m_v b_m_v"></div>

9. bemhtml should duplicate elem elemMod class

({
    block: 'b',
    content: {
        elem: 'e',
        elemMods: { modname: 'modval' },
        mix: { elem: 'e', elemMods: { modname: 'modval' } }
    }
});

But who cares? It’s pretty rare case.
To fix this we need to compare each key/value pairs. It’s too expensive.
I believe that developers should not want to do this.

4.3.4 result:

<div class="b">
    <div class="b__e b__e_modname_modval b__e b__e_modname_modval"></div>
</div>

demo

5.0.0 result:

<div class="b">
    <div class="b__e b__e_modname_modval b__e b__e_modname_modval"></div>
</div>

demo

This PR result:

<div class="b">
    <div class="b__e b__e_modname_modval b__e_modname_modval"></div>
</div>

qfox · 2016-02-15T14:56:54Z

lib/bemhtml/index.js

@@ -213,7 +213,8 @@ BEMHTML.prototype.renderMix = function renderMix(entity,
    if (typeof item === 'string')
      item = { block: item, elem: undefined };

-    var hasItem = item.block || item.elem;
+    var hasItem = (item.block || item.elem) &&
+      !(item.mods && item.block === entity.block);


Pretty scared about this condition. Can we add more tests for edge cases?

Like:

{ block: 'b', elem: 'e', mix: { elem: 'e' } } // 1 { block: 'b', elem: 'e', mix: { block: 'b', elem: 'e' } } // 2 { block: 'b', content: { elem: 'e', mix: { elem: 'e' } } } // 3

@zxqfox it’s defferent cases:

elem with mixed the same elem

elem with mixed elem with elemMods

block with mixed the same block

block with mixed block with mods

Now 1, 2, 3 will render as duplicates but who cares? No degradation with current version, don’t afraid.

This PR about number 4 only. The reason is “I want to mix block (for CSS purposes) with mods without apply templates”.

//cc @tadatuta

Rewrite condition then.
item.elem || item.block && !(item.mods && item.block === entity.block)

qfox · 2016-02-15T18:29:09Z

👍

miripiruni · 2016-02-26T21:50:00Z

I remember this issue and return to PR at monday.

miripiruni · 2016-02-29T23:34:17Z

🆙

This issue is much more interesting than it seems at first sight. See updated PR description.

miripiruni · 2016-03-01T11:01:51Z

I tried to run bench.

prev — 5.0.0.
next — this PR.

miri$ node ./run.js --compare

render:basic:next x 553,115 ops/sec ±0.58% (88 runs sampled)
render:basic:prev x 541,073 ops/sec ±0.92% (87 runs sampled)
render:islands:next x 917 ops/sec ±0.94% (87 runs sampled)
render:islands:prev x 972 ops/sec ±1.38% (86 runs sampled)
render:showcase:next x 1,766 ops/sec ±1.41% (84 runs sampled)
render:showcase:prev x 1,890 ops/sec ±1.38% (86 runs sampled)

JFYI.

tadatuta · 2016-03-01T11:42:16Z

try benches few times. for me it often results in different timing each time I try.

miripiruni · 2016-03-01T11:48:07Z

@tadatuta yes, of course I tried to run it several times. I saw no degradation.

veged · 2016-03-02T14:50:36Z

lib/bemhtml/index.js

+
+    if (item.elem) {
+      if (item.elem !== entity.elem && item.elem !== context.elem) {
+        hasItem = true;


at least hasItem = item.elem !== entity.elem && item.elem !== context.elem

miripiruni · 2016-03-02T18:10:32Z

miri$ node ./run.js --compare --min-samples 1000
render:basic:next x 576,966 ops/sec ±0.98% (1038 runs sampled)
render:basic:prev x 578,712 ops/sec ±0.37% (1037 runs sampled)
render:islands:next x 990 ops/sec ±0.15% (1037 runs sampled)
render:islands:prev x 1,015 ops/sec ±0.20% (1037 runs sampled)
render:showcase:next x 1,939 ops/sec ±0.18% (1036 runs sampled)
render:showcase:prev x 1,908 ops/sec ±0.23% (1037 runs sampled)

qfox · 2016-03-02T23:26:39Z

Nice! Thanks for fixing all these stuff. LGTM

miripiruni · 2016-03-03T09:43:09Z

May I merge?

qfox · 2016-03-03T09:43:46Z

/cc @veged @indutny @arikon

veged · 2016-03-03T13:54:51Z

LGTM

miripiruni · 2016-03-03T13:56:48Z

@veged thank you for your attention! 🎱

Improved mix behaviour

miripiruni added the status: review label Feb 15, 2016

qfox reviewed Feb 15, 2016
View reviewed changes

miripiruni force-pushed the cls branch 4 times, most recently from 517787f to 921b07a Compare February 15, 2016 16:13

miripiruni force-pushed the cls branch from 921b07a to f6208ab Compare February 29, 2016 22:57

miripiruni changed the title ~~Do not repeat block class on mix with same block’s mod~~ Improved mix behaviour Feb 29, 2016

miripiruni force-pushed the cls branch from f6208ab to f62b1d4 Compare February 29, 2016 23:31

miripiruni self-assigned this Feb 29, 2016

miripiruni added type: bug type: enhancement labels Feb 29, 2016

veged reviewed Mar 2, 2016
View reviewed changes

miripiruni force-pushed the cls branch 3 times, most recently from c3b9f4e to 0efadf3 Compare March 2, 2016 18:09

miripiruni added this to the 5.1.0 milestone Mar 2, 2016

miripiruni added the semver: minor label Mar 2, 2016

Improved mix behaviour

c1f2aff

miripiruni force-pushed the cls branch from 0efadf3 to c1f2aff Compare March 2, 2016 19:39

miripiruni added a commit that referenced this pull request Mar 9, 2016

Merge pull request #197 from bem/cls

65ffa96

Improved mix behaviour

miripiruni merged commit 65ffa96 into master Mar 9, 2016

miripiruni removed the status: review label Mar 9, 2016

miripiruni deleted the cls branch March 9, 2016 10:29

miripiruni added the scope: semantic label Mar 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved mix behaviour #197

Improved mix behaviour #197

miripiruni commented Feb 15, 2016

qfox Feb 15, 2016

miripiruni Feb 15, 2016

qfox Feb 15, 2016

qfox commented Feb 15, 2016

miripiruni commented Feb 26, 2016

miripiruni commented Feb 29, 2016

miripiruni commented Mar 1, 2016

tadatuta commented Mar 1, 2016

miripiruni commented Mar 1, 2016

veged Mar 2, 2016

miripiruni commented Mar 2, 2016

qfox commented Mar 2, 2016

miripiruni commented Mar 3, 2016

qfox commented Mar 3, 2016

veged commented Mar 3, 2016

miripiruni commented Mar 3, 2016

Improved mix behaviour #197

Improved mix behaviour #197

Conversation

miripiruni commented Feb 15, 2016

Fixed (degradation)

1. bemhtml should duplicate block class if mix several block with mods to elem in the same block.

Improved

2. bemhtml should not duplicate block class if mix is the same block with mods.

3. bemhtml should not duplicate elem class if mix is the same elem.

4. bemhtml should not duplicate elem class if mix is the same block elem.

5. bemhtml should not duplicate elem class if mix the same elem to elem in block.

6. bemhtml should not duplicate elem class if mix is the same block elem with elemMods.

7. bemhtml should not duplicate block elem elemMods class

“Who cares” cases (zero cost but enjoyable)

8. bemhtml should duplicate block mods class if mix is the same block with mods.

9. bemhtml should duplicate elem elemMod class

qfox Feb 15, 2016

Choose a reason for hiding this comment

miripiruni Feb 15, 2016

Choose a reason for hiding this comment

qfox Feb 15, 2016

Choose a reason for hiding this comment

qfox commented Feb 15, 2016

miripiruni commented Feb 26, 2016

miripiruni commented Feb 29, 2016

miripiruni commented Mar 1, 2016

tadatuta commented Mar 1, 2016

miripiruni commented Mar 1, 2016

veged Mar 2, 2016

Choose a reason for hiding this comment

miripiruni commented Mar 2, 2016

qfox commented Mar 2, 2016

miripiruni commented Mar 3, 2016

qfox commented Mar 3, 2016

veged commented Mar 3, 2016

miripiruni commented Mar 3, 2016