bem / bem-core

BEM Core Library
https://ru.bem.info/technologies/classic/i-bem/
Other
275 stars 94 forks source link

i-bem.bemhtml: Add helpers `addToArray` and `addMixTo` #820

Closed veged closed 8 years ago

veged commented 9 years ago

For avoid copy-paste of [{ block: 'b1' }].concat(this.ctx.mix) we can introduce two new helpers: for construct or convert to array with adding argument after and for inplace modification of mix field.

function addToArray(arr, item) {
    if(typeof arr === 'undefined') arr = [];
    else if(!Array.isArray(arr)) arr = [arr];
    var i = 1;
    while(item = arguments[i++]) arr.push(item);
    return arr
}

function addMixTo(obj, item) {
    var args = Array.prototype.slice.call(arguments, 1);
    args.unshift(obj.mix);
    obj.mix = addToArray(null, args);
    return obj;
}

Also need to think about similar helpers for other fields (like js, attrs and so on).

cc @dfilatov @narqo @mishanga

Update: Discussed with @dfilatov and @narqo — decide to rename addToArray -> addTo

qfox commented 9 years ago

@veged addToArray → push/concat?

mishanga commented 9 years ago

Не вижу смысла увеличивать размер базовых шаблонов. Кроме того, задача выглядит более общей, она бы решилась, например, применением шаблонов к миксам.

veged commented 9 years ago

@zxqfox it's not exactly push even concat, so I prefer more independent name

veged commented 9 years ago

@mishanga это увеличение базовых шаблонов (не такое уж большое) с лихвой компенсируется на проектах за счёт избавления от копипаста

применение шаблонов к миксам это хорошо, но это же не снимет частой задачи "добавить микс"

qfox commented 9 years ago

@veged А не правильнее это реализовать в bh/bemhtml в виде хелперов?

upd: Спрашиваю, потому что https://github.com/bem/bh/blob/master/lib/bh.js#L51-L93 — примерно про то же.

mishanga commented 9 years ago

@zxqfox речь как раз про добавление хелперов. Ни в bemhtml, ни в bh сейчас такого нет. А опыт показывает, что пригодилось бы. Задача отрисовки элементов блока через другой блок оказалось довольно частой.

qfox commented 9 years ago

@mishanga Извините за глупый вопрос, но i-bem.bemhtml означает, что хелперы предлагается на уровне библиотеки? Если так, то не правильнее ли это сделать на уровне библиотек bemhtml (т.е. bem-xjst) и bh?

Честно говоря, вообще не понимаю, почему это обсуждается в рамках i-bem.bemhtml. Хотя начинаю понимать.

feugenix commented 9 years ago

Кстати, а не будет ли логичней реализовать addMixTo так, чтобы она:

  1. не меняла первый аргумент
  2. возвращала новые миксы, а не obj? Я бы, например, использовал addMixTo как-то так:
block test, mix: this.addtoMix(this.ctx, { block: 'mixed' })

и не важно, есть ли в this.ctx миксы или нет.

veged commented 9 years ago

@zxqfox нюанс в том, что в BH нет такого понятия как "базовые шаблоны" (они зашиты в ядре, твоя ссылка про это), а вот для BEMHTML это всё как раз хранится не в ядре (https://github.com/bem/bem-core/blob/v2/common.blocks/i-bem/i-bem.bemhtml#L205)

veged commented 9 years ago

@feugenix я писал на основе реального кода из Серпа, мне показалось, что чаще используется именно примешивание к объекту

а для твоего кейса я подразумевал как раз addToArray:

block test, mix: this.addToArray(this.ctx.mix, { block: 'mixed' })
qfox commented 9 years ago

@veged Да я, как бы, не совсем дурак ;-) Код читать умею. Меня больше интересует, правильно ли это. Может быть лучше в BH реализовать расширения и перенести этот код хелперов в базовые шаблоны? По ощущениям подставлять в this.utils нужный prototype c хелперами не так уж и сложно.

narqo commented 9 years ago

@veged мне больше нраивится this.push() (вместо this.addToArray), потому что есть this.extend() (или this.reapply()) — хочется сохранить консистентность именований.

// TODO: arr может быть `undefined` или не массив
this.push = (arr, ...items) => Array.prototype.push.apply(arr, items)

addToMix выглядит слишком частным костылем, IMHO.

qfox commented 9 years ago

Предлагаю суффикс To для всех методов, которые первым аргументом принимают контекст (массив/объект), т.е.: pushTo или arrayAddTo, и addMixTo (как и было предложено). concatTo, на мой взгляд, более понятно, чем push/pushTo, потому что у [].push другой интерфейс, когда concat не меняет исходный объект и возвращает массив.

block test, mix: this.concatTo(this.ctx.mix, { block: 'mixed' })
block test, mix: this.union(this.ctx.mix, [{ block: 'mixed' }]) // lo-dash style

Можно попробовать разложить эти хелперы на более мелкие. Например,

block test, mix: this.toArray(this.ctx.mix).concat({ block: 'mixed' })
veged commented 9 years ago

@zxqfox про BH надо обсуждать с @mishanga ;-)

mishanga commented 9 years ago

В BH такой проблемы нет.

veged commented 9 years ago

Discussed with @dfilatov and @narqo — decide to rename addToArray -> addTo

veged commented 9 years ago

@mishanga как нет такой проблемы, если BEMJSON везде одинаковый? есть кейсы когда приходит неизвестный кусок и в него надо добавить миксы

mishanga commented 9 years ago

@veged в BH метод mix уже умеет резолвить такое: https://github.com/bem/bh/blob/master/lib/bh.js#L345

veged commented 9 years ago

@mishanga этот метод только про текущий контекст — он не поможет при построении bemjson (например при генерации вложенного контента)

qfox commented 9 years ago

@mishanga

if (mix === undefined) {
    return this.ctx.mix;
}

if (force || !this.ctx.mix) {
    this.ctx.mix = mix;
} else {
    this.ctx.mix = Array.isArray(this.ctx.mix) ?
        this.ctx.mix.concat(mix) :
        [this.ctx.mix].concat(mix);
}
return this;

;-(

tadatuta commented 8 years ago

Closing in favour of https://github.com/bem/bem-xjst/issues/242