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

Fix unbind on block destruct #1581

Open
wants to merge 1 commit into
base: v4
Choose a base branch
from
Open

Conversation

belozer
Copy link
Member

@belozer belozer commented Jun 20, 2018

fix #1580

@@ -250,9 +250,11 @@ var undef,
ctxStorage = eventStorage[ctxId] = {};
if(isBindToInstance) {
ctx._events().on({ modName : 'js', modVal : '' }, function() {
params.bindToArbitraryDomElem && ctxStorage[storageKey] &&
Copy link
Member Author

@belozer belozer Jun 20, 2018

Choose a reason for hiding this comment

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

@veged может bindToArbitraryDomElem полностью из параметров удалить? Он использовался только в этом месте.

Copy link
Member Author

Choose a reason for hiding this comment

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

Этот параметр появился после этого фикса d095501

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 Author

Choose a reason for hiding this comment

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

Если @veged не против, то добавлю. В переписке телеги были сомнения на этот счёт.

Copy link
Member

Choose a reason for hiding this comment

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

я не понимаю, почему хочется его удалять? это же про ветку, когда мы биндимся к произвольной jQuery-цепочке

Copy link
Member Author

@belozer belozer Jun 22, 2018

Choose a reason for hiding this comment

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

@veged потому что оно не используется. Точнее использовалось непонятно для чего.

При каждой новой связке через _events() создаётся новый мендежер для текущего контекста.
При этом подписка на отписку происходит только при первой связке и то если имеет этот ключ.

Copy link
Member

Choose a reason for hiding this comment

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

«используется непонятно для чего» != «не используется» ;-)

я ж говорю, bindToArbitraryDomElem используется при байнде к произвольной jQuery-цепочке

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Если в планах использовать какие-то кейсы опираясь на этот флаг, тогда всё ок )

Copy link
Member

Choose a reason for hiding this comment

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

так тут и было это использование — я поэтому и не понимаю, как ты смог его удалить ;-) нет спек на байнд к произвольной jQuery-цепочке получается :-(

Copy link
Member Author

Choose a reason for hiding this comment

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

@coveralls
Copy link

coveralls commented Jun 20, 2018

Coverage Status

Coverage increased (+0.02%) to 63.032% when pulling 1805502 on belozer:issues/1580@v4 into 9ab08cc on bem:v4.

@belozer belozer force-pushed the issues/1580@v4 branch 2 times, most recently from d1c8214 to a978ac7 Compare June 22, 2018 00:05
@tadatuta tadatuta requested a review from veged June 22, 2018 08:56
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