[ns.Events] Неверно работает ns.Events.once #607
Conversation
vitkarpov
changed the title
| var that = this; | ||
| var once = function() { | ||
| that.off( name, once ); | ||
| handler.apply(this, arguments); | ||
| }; | ||
| this.on( name, once ); | ||
| once.__original = handler; |
There was a problem hiding this comment.
Мне кажется, что при отписке необходимо затирать эту ссылку:
once.__original = null;а то создадим утечку памяти
There was a problem hiding this comment.
Не думаю.
Исходная функция не уйдёт раньше, чем once, так как на неё будет как минимум одна актуальная ссылка. Если она нигде больше не нужна, то она уйдет в следующем событии вместе с once. Если нужна, то просто будет меньше на одну ссылку на неё (так как уйдет once).
|
👍 |
|
Круто, но для полноты надо поддержать двойной |
|
С двойным |
|
@chestozo проблемы не будет. const say = () => console.log('yo')
ns.events.once('yo', say)
ns.events.on('yo', say)
ns.events.off('yo', say) // снимет обработчик once, а не onCошлись на том, что с этим можно жить и достаточно описать эту особенность в документации. Что думаете? |
|
А какое поведение ожидаемое в данном случае? Кажется, что должен снять оба обработчика, да? |
|
@vitkarpov в том-то и дело, что любое будет неожиданным. Раньше А вообще я с трудом представляю зачем может быть надо добавать одну и ту же функцию дважды. |
|
Ну, т.е. такой ситуации, как ты описал, по идее, не должно быть? |
|
В реальной жизни |
|
Не должно. Поэтому я и предложил не пытатья её решить, а просто задокументировать, что поведение будет странным и что не нужно добавлять одну и ту же функцию в обработчики событий дважды |
|
UPD: а, нет, не будет |
|
Ну и ок 👍 |
|
👍 |
к примеру, по ошибке @iEgit:
|
Если только в DEV-сборке (как react и т.п.) |
Есть ns.log.error — он как раз так и работает |
|
@iEgit ping? поправишь по комментариям, докинешь тестов:
|
|
да, доделаю |
А на что проверку? На то, что уже есть такой обработчик где-то? |
|
тесты добавил |
|
👍 @chestozo ? |
| // Нашли и удаляем этот обработчик. | ||
| handlers.splice(i, 1); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Т.е. такие нельзя два раза навесить один и тот же обработчик?
Мне кажется, надо делать как в jquery, а там можно:
var $body = $(document.body);
var handler = function() {
console.log(+new Date());
};
$body.one('click', handler);
$body.one('click', handler);
$body.trigger('click');
// 1468415760081
// 1468415760082There was a problem hiding this comment.
Я думаю мы ориентируемся на jquery. var $body = $(document.body);
var handler = function() {
console.log(+new Date());
};
$body.one('click', handler);
$body.on('click', handler);
$body.off('click', handler);
$body.trigger('click');
// Ничего не происходит |
|
Может допилим до поведения "как в jquery"? |
|
А ты уверен, что нужно ориентироваться на эту великую технологию? |
Вообще, кажется, логичным чтобы Т.е. должно быть аналогично случаю: foo.on('event', handler);
foo.on('event', handler);
foo.off('event', handler);Сейчас это не так? |
Пока используется jquery - лучше бы поведение наших эвентов было бы максимально аналогичным.
А я не знаю, давайте напишем тест :) |
https://github.com/yandex-ui/noscript/pull/607/files/a2917b0895c83c97132382849dd15099b23e680a#diff-99f6436afe06f3898363ed5e9798b602R144 — а вот есть тест на двойной Работает консистентно с текущим поведением
Они сейчас все хранятся в одном массиве плоским списком, как только нашли первый handler, то удаляем его из массива и делаем Мы не можем сделать поведение как в jquery, мы должны сделать как в |
|
Поговорили голосом - завёл #613 |
ns.events.off не отписывает once события.
Пример
Добавил ссылку на оригинальный обработчик и поправил off.