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

V2: Model events during "remove" do not trigger on Col #3806

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

Conversation

jridgewell
Copy link
Collaborator

DO NOT MERGE, BREAKING CHANGE This is for the next major, v2.

On the heels of #3803, this prevents events triggered on the model during the "remove" event listener from bubbling up to the collection.

Why? Because the model is "removed" from the collection. You can't #get it any more, there're no more references. The collection shouldn't know anything about the model once that event is fired.

@jashkenas
Copy link
Owner

That makes perfect sense. You really don't think this should be merged?

@jridgewell
Copy link
Collaborator Author

Not now, it's a breaking change. I figure if this many people are causing an infinite loop in the "remove" event, they're also relying on event bubbling.

@platinumazure
Copy link
Contributor

The remove event itself is still fired on the collection, though, right?

@jridgewell
Copy link
Collaborator Author

Yes.


if (!options.silent) {
options.index = index;
model.trigger('remove', model, this, options);
this._onModelEvent('remove', model, this, options);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why call _onModelEvent here? We know it will bypass all of the conditionals because we don't specifically handle remove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since it could be overridden. We're not exactly clear on what is overridable and what's not. #3703

Copy link
Collaborator

Choose a reason for hiding this comment

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

:eyeroll:

The ones that are meant to be overridden are usually obvious, or their
utility becomes clear when you need them. They don't get in your way if you
don't need them.

I can't think of a reason why you'd override _onModelEvent. It seems to be
there mostly to make it easier to unbind later.

Theres no issue using _onModelEvent but I like the symmetry of the two
triggers in a row.

@jridgewell jridgewell changed the title Model events during "remove" do not trigger on Col V2: Model events during "remove" do not trigger on Col Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants