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

Change to splice method so it is observable #4027

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

Conversation

robinchesterman
Copy link

@robinchesterman robinchesterman commented May 3, 2016

The existing splice method uses the indexer:

array[i + at] = insert[i]

This is not a natively observable change using modern Javascript Reflection APIs.

This PR changes the method to achieve the same effect but using native Array.splice.

This may be useful when using Backbone with other frameworks e.g. it may be desirable to use Backbone modelling for the excellent collection methods and REST APIs, but Aurelia or Angular for UI data-binding.

The latter frameworks can't observe changes on collection.models array if they're done with the indexer. Splice is observable.

@megawac
Copy link
Collaborator

megawac commented May 6, 2016

Splice was added in #3663. I'll defer to @jridgewell on this one. If this does get in, I'd rather remove the internal splice function all togehter

@jridgewell
Copy link
Collaborator

I removed splice because of its horrible performance in loops (adding an array of models at a particular index).

How is #splice observable bot indexed sets aren't?

@jgonggrijp
Copy link
Collaborator

I'm in favor of this change, but the performance claim needs quantification before the change can be made. By "quantification", I mean something a little more serious than a nanobenchmark on jsperf. Not to mention that jsperf seems to be broken.

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.

4 participants