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

i-bem: Add _ to all @protected methods #586

Closed
veged opened this issue Jul 9, 2014 · 19 comments
Closed

i-bem: Add _ to all @protected methods #586

veged opened this issue Jul 9, 2014 · 19 comments
Assignees
Labels
Milestone

Comments

@veged
Copy link
Member

veged commented Jul 9, 2014

No description provided.

@qfox
Copy link
Member

qfox commented Jan 23, 2015

В общем, npm run lint в тревисе не запускается, но в #749 оно проходит ок, кроме тех случаев, где указан @protected в jsdoc и отсуствует _ в названии функции.

Нужен ревью.

@veged
Copy link
Member Author

veged commented Feb 17, 2015

у нас сейчас ошибки вида Missing leading underscore for init at common.blocks/i-bem-dom/i-bem-dom.js : даже там, где не указан @protected

@qfox
Copy link
Member

qfox commented Feb 17, 2015

@veged А кинь ссылкой в код?

@veged
Copy link
Member Author

veged commented Feb 17, 2015

ветка issues/394@v3

@qfox
Copy link
Member

qfox commented Feb 17, 2015

@veged Баг, если не указан @access оно считает, что надо репортить, поправил в 0.4.5

@qfox
Copy link
Member

qfox commented Feb 17, 2015

[...elems] — а такое нужно, по-хорошему, в типе указывать: http://usejsdoc.org/tags-param.html#multiple-types-and-repeatable-parameters

@arikon
Copy link
Member

arikon commented Apr 27, 2015

@veged Почему вы решили добавлять _ к защищённым методам? Они же, по сути, являются частью публичного API в терминах semantic versioning (можно доопределять на сервисе; нужно гарантировать сигнатуру неизменной).

@qfox
Copy link
Member

qfox commented Apr 27, 2015

Если нужны только приватные — можно выставить "leadingUnderscoreAccess": private в .jscsrc

@arikon Главное без фанатизма к ООП подходить, наследование второго уровня — пол беды, третьего уровня — беда ;-(

@veged
Copy link
Member Author

veged commented Apr 28, 2015

_ показывает, что этот метод нельзя внутри своего кода вызывать

@qfox
Copy link
Member

qfox commented Apr 28, 2015

В общем, у нас нет @protected. Только @private и @public. чтоли?

@narqo
Copy link
Member

narqo commented Apr 28, 2015

@zxqfox on (bindTo и остальные методы про подписку на события), emit@protected-методы, которые «нельзя» вызывать у инстанса, только у себя (this).

@qfox
Copy link
Member

qfox commented Apr 28, 2015

@narqo Всмысле, у класса? https://github.com/bem/bem-core/blob/v2/common.blocks/i-bem/__dom/i-bem__dom.js#L1186-L1332 — статические вот эти? Тогда я за префикс/постфикс _

@narqo
Copy link
Member

narqo commented Apr 29, 2015

Нет, у инстанса, в первую очередь:

this.on('click', …) // — нормально
this.findBlockInside('link').bindTo('blah') // — не очень
this.findBlockOutside('header').emit('open') // — не очень

@qfox
Copy link
Member

qfox commented Apr 29, 2015

this.findBlockInside('link').bindTo('blah') // — не очень

а как? скажем, есть у нас menu — у него внутри menu-item разные или элементы:

this.findBlockInside('menu-item').onBlah(...);

DOM.decl('menu-item', {
  onBlah: function (cb) {
    this.on('blah', cb);
  }
});

Так чтоли?

Я не очень понимаю, почему они должны быть @protected.

Case 2:

BEM.decl('someEmitter', {
  _onBlah: function (cb) {
    this.emit('blah', cb);
  }
});

Как ловить это событие?

@narqo
Copy link
Member

narqo commented Apr 29, 2015

Для первого случая, в v3 мы делаем event-manager. Через него ты свой блок будешь подписывать на события чужого блока Сейчас ты вынужден подписывать чужой блок на его же события, передавая свои хендлеры — это ведет к необходимости помнить про отписку, в случае твоего деструкта (и это «не очень»)

Второй кейс даже в v2 выглядит надуманным, зачем тебе BEM-блок? evens.Event не подойдет?

@qfox
Copy link
Member

qfox commented Apr 29, 2015

Второй кейс даже в v2 выглядит надуманным, зачем тебе BEM-блок? evens.Event не подойдет?

Вполне обычный кейс: input.on('change', ...);

Для первого случая, в v3 мы делаем event-manager. Через него ты свой блок будешь подписывать на события чужого блока Сейчас ты вынужден подписывать чужой блок на его же события, передавая свои хендлеры — это ведет к необходимости помнить про отписку, в случае твоего деструкта (и это «не очень»)

В таком случае я вообще не понимаю.
this.findBlockInside('link').bindTo('blah') // — не очень а это нельзя через event-manager делать?

@narqo
Copy link
Member

narqo commented Apr 29, 2015

Вполне обычный кейс: input.on('change', ...);

У тебя в примере BEM.decl (не BEMDOM.decl). Для BEMDOM-блоков будет event-manger для подписки на BEM- и DOM-события. Подробности в #394

Давай сразу перестанем тут придумывать кейсы «как отстрелить себе что-то». Любые задачи можно решить в рамках текущего API (и даже в рамках API bem-bl, при желании). Задача: сделать API, которое позволит решать пользователю те же задачи, не держа в голове кучу факторов, которые потом «аукаются в продакшне»

@qfox
Copy link
Member

qfox commented Apr 29, 2015

@narqo Давай не будем. Тогда если я сам вешаю обработчик — я сам его должен снимать. И в этом случае, проще оставить их @public и не приписывать _. И тогда все логично ;-)

narqo pushed a commit that referenced this issue Jun 11, 2015
@narqo narqo closed this as completed Jun 11, 2015
@narqo narqo removed the in progress label Jun 11, 2015
@narqo
Copy link
Member

narqo commented Jun 11, 2015

done with #1046

@narqo narqo changed the title Add _ to all @protected methods i-bem: Add _ to all @protected methods Sep 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants