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

Collection.remove behavior in 1.2.X #3847

Closed
gaperton opened this issue Nov 3, 2015 · 4 comments
Closed

Collection.remove behavior in 1.2.X #3847

gaperton opened this issue Nov 3, 2015 · 4 comments

Comments

@gaperton
Copy link

gaperton commented Nov 3, 2015

In previous versions, when model was removed from collection, you first cleaned up the index (this._byId), and only then you trigger remove event. Which was okay.

In 1.2, you first remove the model, trigger remove, and only then clean up the index in _removeReference. Thus, in the remove event handler model is still present in the index (while absent in collection), resulting in successful get. Which leads to weird things, like infinite loop when trying to remove the same model again in 'remove' event handler.

Is there any specific reason you now triggering 'remove' while collection is in inconsistent state, or it's just a bug?

    _removeModels: function(models, options) {
      var removed = [];
      for (var i = 0; i < models.length; i++) {
        var model = this.get(models[i]);
        if (!model) continue;

        var index = this.indexOf(model);
        this.models.splice(index, 1);
        this.length--;

        if (!options.silent) {
          options.index = index;
          model.trigger('remove', model, this, options);
        }

        removed.push(model);
        this._removeReference(model, options);
      }
      return removed.length ? removed : false;
    },
@akre54
Copy link
Collaborator

akre54 commented Nov 3, 2015

Are you referring to #3693 and #3803?

@gaperton
Copy link
Author

gaperton commented Nov 3, 2015

Yes, I guess it's the same.

@akre54
Copy link
Collaborator

akre54 commented Nov 3, 2015

Cool. Feel free to weigh in there.

@akre54 akre54 closed this as completed Nov 3, 2015
@gaperton
Copy link
Author

gaperton commented Nov 4, 2015

Thanks for invitation. Quite recently I have decided that I don't need dependency from backbone in my framework any more. I would rather have my own implementation of collections. Have fun with tickets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants