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.

Sign up for GitHub

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

Jump to bottom

Fix unbind on block destruct #1581

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

Fix unbind on block destruct #1581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 30 additions & 8 deletions common.blocks/i-bem-dom/__events/_type/i-bem-dom__events_type_bem.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ describe('BEM events', function() {
});

describe('block instance events', function() {
var block2_1, block2_2;
var block1, block2_1, block2_2, block3_1, block3_2;
beforeEach(function() {
Block1 = bemDom.declBlock('block1', {
onSetMod : {
Expand All @@ -222,14 +222,27 @@ describe('BEM events', function() {
});

Block2 = bemDom.declBlock('block2');
Block3 = bemDom.declBlock('block3');

block1 = initDom({
block : 'block1',
content : [
{ block : 'block2' },
{ block : 'block2' }
]
}).bem(Block1);
var dom = initDom([
{
block : 'block1',
content : [
{ block : 'block2' },
{ block : 'block2' }
]
},
{ block : 'block3' },
{ block : 'block3' },
]);

block1 = dom.eq(0).bem(Block1);

block3_1 = dom.eq(1).bem(Block3);
block3_2 = dom.eq(2).bem(Block3);

block1._events(block3_1).on('click', spy6);
block1._events(block3_2).on('click', spy7);
});

it('should properly bind handlers', function() {
Expand Down Expand Up @@ -267,6 +280,15 @@ describe('BEM events', function() {
spy1.should.not.have.been.called;
spy3.should.have.been.called;
});

it('should properly unbind all handlers on block destruct', function() {
bemDom.destruct(block1.domElem);
block3_1._emit('click');
block3_2._emit('click');

spy6.should.have.not.been.called;
spy7.should.have.not.been.called;
});
});

describe('nested blocks events', function() {
Expand Down
2 changes: 2 additions & 0 deletions common.blocks/i-bem-dom/__events/_type/i-bem-dom__events_type_dom.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,9 +745,11 @@ describe('DOM events', function() {
it('should properly unbind all handlers on block destruct', function() {
bemDom.destruct(block1.domElem);
rootNode.trigger('click');
rootNode.trigger('dblclick');

spy1.should.not.have.been.called;
spy2.should.not.have.been.called;
spy4.should.not.have.been.called;
});
});
});
Expand Down
7 changes: 4 additions & 3 deletions common.blocks/i-bem-dom/__events/i-bem-dom__events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,10 @@ 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 •
edited
Loading

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 •
edited
Loading

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.

Разве эти тесты не покрывают? https://github.com/bem/bem-core/blob/v4/common.blocks/i-bem-dom/__events/_type/i-bem-dom__events_type_dom.spec.js#L665-L753

ctxStorage[storageKey].un();
delete ctxStorage[ctxId];
objects.each(ctxStorage, function(eventManager) {
eventManager.un();
});
delete eventStorage[ctxId];
});
}
}
Expand Down