Wrap data-bem attribute by single quotes

[qfox]

Fixes #114 Closes gh-115

[coveralls]

Coverage remained the same at 99.74% when pulling 48f46149985cf359b8bf1198b3145d0e1311e816 on zxqfox:issues/114 into b02c008a6723e86ebbfc148addcdb5d9fc4988e1 on bem:master.

[coveralls]

Coverage remained the same at 99.74% when pulling 78f50682df19e31ea78afc5aef14131e2ead5a3f on zxqfox:issues/114 into b02c008a6723e86ebbfc148addcdb5d9fc4988e1 on bem:master.

[mishanga]

/cc @denchistyakov @sameoldmadness

[sameoldmadness]

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

[qfox]

@sameoldmadness В результирующем коде получается меньше буковок. Вместо:

data-bem="{"a":{},"b":{}}"

Получаем:

data-bem='{"a":{},"b":{}}'

+. Проще читать параметры в исходниках.

+. Это синхронизация с https://github.com/bem/bem-core/pull/742#issuecomment-72166752 — иначе тесты будут падать.

В любом случае, надо дождаться влития 742 из бем-кор, иначе опять «тесты будут падать», назвать вменяемо (может в attrEscape унести: типа attrEscape(jsAttr, true)), и еще можно сделать это настраиваемо через setOptions (тогда можно не дожидаться 742).

[sameoldmadness]

меньше буковок

Рискну предположить, что на пожатых gzip'ом документах разница будет незаметна. Как минимум, стоит замерить.

Проще читать параметры в исходниках.

Chrome Dev Tools решают эту задачу. Или исходники смотрят как-то иначе?

Это синхронизация с bem/bem-core

Можно поднять вопрос, нужно ли это в bem-core.

Я почему пристаю: на мой взгляд, нужна очень веская причина, чтобы начать обрамлять значение атрибута одинарными кавычками.

И для того, чтобы заменить функцию экранирования, кстати, тоже.

[qfox]

Рискну предположить, что на пожатых gzip'ом документах разница будет незаметна. Как минимум, стоит замерить.

Никто не спорит ;-) разница — пара байт в реальных условиях даже с большими кусками.

Chrome Dev Tools решают эту задачу. Или исходники смотрят как-то иначе?

Ща научу: curl -v http://ya.ru/

Можно поднять вопрос, нужно ли это в bem-core.

https://github.com/bem/bem-core/issues/250 — это вам к туда ;-)

Я почему пристаю: на мой взгляд, нужна очень веская причина, чтобы начать обрамлять значение атрибута одинарными кавычками.

Вегед — достаточно веская причина? ;-) Наверное, это что-то может сломать, если ты так этого боишься. Хотелось бы узнать, конечно.

И для того, чтобы заменить функцию экранирования, кстати, тоже.

Ну как, это же только для jsAttr, не вижу, что это может сломать. Данные внутри — это json, либо return + json.

Единственное, что это может сломать — какие-то внешние сервисы, которые парсят html нестандартно, и завязаны на эти кавычки и " и на data-bem/onclick атрибут. Это обратная ситуация к «смотреть исходники» и для страждущих — можно сделать это опционально, выставляя обратное экранирование через setOptions.

[qfox]

@sameoldmadness Вообще, я согласен, что менять поведение по умолчанию — идея плохая.

Я предлагаю

• Отменить все изменения тестов;
• Добавить булевую опцию jsAttrSingleQuoted (или как-то еще, предлагайте), которая будет менять кавычки;
• Добавить булевый параметр в attrEscape, который использовать только в jsAttr;
• Добавить тесты на эту опцию.

Это ничего не сломает, не потребует мажорного апдейта версии, и не потребует дальнейшей поддержки.

[sameoldmadness]

Ща научу: curl -v http://ya.ru/

Ну и много ты там насмотришь, в минифицированном-то коде без подсветки? : )

Вегед — достаточно веская причина?

Конечно, нет. Но у него наверняка есть веская причина. Задал вопрос.

Наверное, это что-то может сломать, если ты так этого боишься. Хотелось бы узнать, конечно.

Страшно боюсь всего, что может что-то сломать, и не добавляет при этом ценности.

Добавить булевую опцию

Лучше, чем менять поведение по умолчанию, но:

• больше кода
• больше опций
• больше тестов

[qfox]

В общем, если вводить — то вводить опцией, имхо. На самом факте ввода я не настаиваю, но было бы полезно.

Лучше, чем менять поведение по умолчанию, но:

Да там не много кода и 1 тест. И в будущем с этим проблем не будет — оно локальное, тестировать не надо. Тесты можно продолжать писать со старым поведением.

[mishanga]

@sameoldmadness идея действительно в том, чтобы сгенерированный код стал чище. В плане количества байтиков это действительно не даст ощутимого профита, но читать исходники и писать тесты станет сильно удобнее.

В dev tools совсем недавно появилась фича автоматического де-эскейпинга, раньше показывалось как есть: data-bem="{"ololo":{}}"

А исходники действительно иногда приходится читать, чтобы исключить влияние JS и браузера. Обычно, конечно, исходники читают прямо в браузере, чтобы была подсветка, но изредка приходится и в консоли смотреть. Хотя и там можно открыть html в vim с подсветкой.

Что касается "поведения по умолчанию", то это не совсем корректный аргумент, т.к. эскейпинг и формат выходных данных инкапсулирован и никак не регламентируется. Требование одно: он должен корректно работать во всех браузерах. Эти изменения повлияют только на юнит-тесты на шаблоны, но не затронут работу самих шаблонов и клиентских скриптов.

@zxqfox Кстати, я бы лучше добавил отдельную функцию типа jsAttrEscape для её последующего использования при генерации кастомных data-атрибутов. Это позволит не трогать существующее API.

[qfox]

@mishanga Нууу...

Что касается "поведения по умолчанию", то это не совсем корректный аргумент, т.к. эскейпинг и формат выходных данных инкапсулирован и никак не регламентируется.

Если меняется поведение — это тянет на мажорный релиз. Имхо, нет проблем поменять поведение по умолчанию на «более чистое» вместе с какими-то другими крупными необратимыми изменениями, а людям жить проще.

Эти изменения повлияют только на юнит-тесты на шаблоны

И если у меня их 150 и я не умею sed s/"/'"'/ -i, то у меня будет боль. ;-(

Кстати, я бы лучше добавил отдельную функцию типа jsAttrEscape для её последующего использования при генерации кастомных data-атрибутов.

Да не вопрос, я не трогал PR, потому что не понятно что будет в bem-core, и если и тут и там будет jsAttrEscape — будет вдвойне ок.

[mishanga]

@zxqfox сделай rebase от https://github.com/bem/bh/compare/singlequot

[qfox]

@mishanga done.

[mishanga]

@sameoldmadness @denchistyakov @dfilatov коллеги, всех устраивает такое решение?

[qfox]

@mishanga Лучше за опцию.

[sameoldmadness]

@mishanga В отсутствие варианта "ничего не менять" — устраивает.

[mishanga]

@zxqfox еще забыли добавить тесты на jsAttrEscape в test.escape.js.

[coveralls]

Coverage increased (+0.0%) to 99.73% when pulling d3aed57fed92768b2ad14bf1ac54eb9ba1c617f1 on zxqfox:issues/114 into c2719a954e5f5ab67ea1d158cbfd53d9821a0bf4 on bem:master.

[qfox]

@mishanga Добавил тест и поправил \\\'

[Yeti-or]

@mishanga @zxqfox Так это будет патч версия?

[qfox]

@YetiOr Я не в восторге, но я не мейнтейнер ;)

[mishanga]

@YetiOr @zxqfox не-не, минимум минорная версия.

[mishanga]

@zxqfox :tada:

[qfox]

:balloon: