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

Issue #168: implement content escaping (take 2) #217

Merged
merged 5 commits into from
Apr 20, 2016
Merged

Conversation

miripiruni
Copy link
Contributor

@miripiruni miripiruni commented Mar 15, 2016

Instead of #178

  • Docs
  • escapeContent flag
  • Special content value: { html: '…' }
  • Tests
  • Benchmarks

Benchmarks

results for 10000 runs:

average diff 1% (from 1μs) slower on apply() and the same on compile()

compile:granny:next x 596 ops/sec ±0.36% (10027 runs sampled)
compile:granny:prev x 601 ops/sec ±0.35% (10030 runs sampled)
–0.83% от 1ms

render:granny:next x 1,153,802 ops/sec ±0.23% (10033 runs sampled)
render:granny:prev x 1,257,028 ops/sec ±0.20% (10033 runs sampled)
–8,2% от 1μs


compile:islands:next x 192 ops/sec ±0.30% (10023 runs sampled)
compile:islands:prev x 190 ops/sec ±0.30% (10023 runs sampled)
~ the same

render:islands:next x 433 ops/sec ±0.19% (10029 runs sampled)
render:islands:prev x 442 ops/sec ±0.18% (10030 runs sampled)
–1,58% от 1ms


compile:serp:next x 25.51 ops/sec ±0.61% (10015 runs sampled)
compile:serp:prev x 24.86 ops/sec ±0.96% (10016 runs sampled)
~ the same

render:serp:next x 1,146,380 ops/sec ±0.55% (10033 runs sampled)
render:serp:prev x 1,169,145 ops/sec ±0.56% (10032 runs sampled)
–1,94% от 1μs 

Islands & shocase bemhtml outdated.

The plan:

  1. minor release with escapeContent:false option.
  2. major release with escapeContent:true option.

test(function() {
}, [ { html: [ '<danger>' ] },
{ html: { toString: function () { return '<lol>'; } } } ],
'<div></div><div></div>');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хотя, если не обрабатывается toString, то там можно вывести блоки без экранирования? Так, чтоли?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zxqfox nope.

{
    html: { block: 'test', content: 'H&M' }
}

Will treated as:

{}
<div></div>

because of typeof html !== 'string' therefore html — will treated as user defined field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Black magic.

👎

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

если мы не хотим разрешать в html ничего кроме строк, то нужно явно кидать ексепшен в этом месте, а не делать так, чтобы объекты с toString рендерились не в строку

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

я так и не понял, что мы делаем для кейсов, когда там не строка, а объект? точно мы не хотим бросать эксепшен??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У нас люди не очень любят эксепшны. Мы можем подумать в сторону варнингов, чтобы писать про такое в логи, например.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Дело не в том, что любят люди :)

Просто я бы не стал никак делать лишних ограничений. Есть поведение задекларированное и мы будем его поддерживать (передавай строку), а всё остальное не гарантируется, используй на свой страх и риск.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вообще, я за как можно меньше доп логики в шаблонизаторе — нет смысла в лишних проверках, жрущих CPU и требующих документации. Проверка на какие-то поля block/elem/tag/content (?) кажется вполне разумным компромисом, учитывая что мы не можем проверить просто html, потому что это кому-то что-то может сломать (мажор может просто?).

@veged
Copy link
Member

veged commented Mar 17, 2016

Big question about benchmarks! Now they are go to dissemble («a little bit») because in BEMJSON there is not so many text nodes. basic.bemjson more over is too small even for any other benchmarks.

I suggest to add one more example similar to SERP in terms of total amount of different blocks (for other benchmarks) and in terms of proportions between BEM-entities and their text content (for this particular case of benchmarking).

@@ -104,6 +104,23 @@
}
```

Объект с полем `html` является специальным значением поля `content`. Все другие
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

скорее «является специальным типом возможных объектов в BEMJSON» — как минимум, потому, что я могу написать

{
  block: 'b1',
  content: [
    '1',
    { html: '2' },
    '3'
  ]
}

и это уже не будет всем значением поля content

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

html является специальным полем объекта в BEMJSON

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mishanga Это в bh ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

скорее «является специальным типом возможных объектов в BEMJSON»

Fixed.

@miripiruni
Copy link
Contributor Author

Rebased from master.

@miripiruni miripiruni force-pushed the escaping2 branch 3 times, most recently from c2102ab to d10b854 Compare April 8, 2016 14:39
@miripiruni
Copy link
Contributor Author

To all: so, let’s merge it today?

@veged
Copy link
Member

veged commented Apr 19, 2016

can't catch what exactly numbers for bench results?

@miripiruni
Copy link
Contributor Author

miripiruni commented Apr 19, 2016

@veged depends on project. But average diff +1% on apply() and the same on compile(). See PRs description.

@veged
Copy link
Member

veged commented Apr 19, 2016

please, put this in clearly description ;-) and merge

@miripiruni
Copy link
Contributor Author

Description updated.

@miripiruni miripiruni added this to the 6.4.0 milestone Apr 20, 2016
@miripiruni miripiruni merged commit 9bdb204 into master Apr 20, 2016
@qfox qfox deleted the escaping2 branch April 20, 2016 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants