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

Do not alter the "on" events array #3463

Merged
merged 1 commit into from
Mar 4, 2015
Merged

Do not alter the "on" events array #3463

merged 1 commit into from
Mar 4, 2015

Conversation

jridgewell
Copy link
Collaborator

No description provided.

@jridgewell jridgewell mentioned this pull request Jan 27, 2015
Merged
@akre54
Copy link
Collaborator

akre54 commented Jan 28, 2015

What does this give us?

@akre54 akre54 added the change label Jan 28, 2015
@jridgewell
Copy link
Collaborator Author

The test was a false positive. "bind does not alter callback list" and "unbind does not alter callback list".

@akre54
Copy link
Collaborator

akre54 commented Jan 28, 2015

Couldn't we also slice on L163 (or in triggerEvents, etc)? What's the benefit to re-assigning the events hash?

@jridgewell
Copy link
Collaborator Author

Couldn't we also slice on L163

Yup. I think the real point of these tests was to make sure offing didn't cause an array out of bounds error.

@akre54
Copy link
Collaborator

akre54 commented Jan 28, 2015

Totally, and agreed. Can we just add the tests without making changes at the moment, or are you saying the tests would be broken too? If that's the case my personal preference would be to slice over the array in trigger, rather than in on, since it just feels more correct. this._events isn't immutable, and we really shouldn't be reassigning it every time on is called.

@braddunbar
Copy link
Collaborator

Yup. I think the real point of these tests was to make sure offing didn't cause an array out of bounds error.

Close, but not quite. The point of the test is to make sure off doesn't mutate the array in a way that causes specific event handlers to be skipped (a hard fought debugging session!). Adding events to the end of the handler list is fine. I imagine this would have some fairly serious performance ramifications as well.

Couldn't we also slice on L163 (or in triggerEvents, etc)? What's the benefit to re-assigning the events hash?

I'd prefer this approach if it's an issue. It was once done this way and I don't remember why it changed. 😕

@jridgewell
Copy link
Collaborator Author

The tests would be broken if this were merged without the concat. Though, that would be because the test's purpose isn't what it is actually testing.

@jridgewell
Copy link
Collaborator Author

The point of the test is to make sure off doesn't mutate the array in a way that causes specific event handlers to be skipped (a hard fought debugging session!).

That would make sense too 😄. Let me look into this again.

@braddunbar
Copy link
Collaborator

Coolio. Thanks @jridgewell! 😃

@jridgewell jridgewell mentioned this pull request Jan 28, 2015
Closed
@jridgewell jridgewell force-pushed the on-does-not-alter branch 2 times, most recently from a61ddb4 to 5e38324 Compare January 28, 2015 20:28
@jridgewell
Copy link
Collaborator Author

Ping. Let me know how you feel about it now.

It's just a cleanup of the test. My concat was just preventing allEvents from being mutated by any handlers in events, but I don't think that's necessary?

If it is, we would just have to allEvents.slice() before L165.

jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 5, 2015
@jridgewell
Implement Events as a linked list.
fdea387 
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks.

This fixes jashkenas#3466 and closes jashkenas#3463.
jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 5, 2015
@jridgewell
Implement Events as a linked list.
49b3287 
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
@jridgewell jridgewell mentioned this pull request Feb 5, 2015
Closed
jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 7, 2015
@jridgewell
Implement Events as a linked list.
394370d 
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 7, 2015
@jridgewell
Implement Events as a linked list.
cc593b5 
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
jashkenas added a commit that referenced this pull request Mar 4, 2015
@jashkenas
Merge pull request #3463 from jridgewell/on-does-not-alter
dfec4b8 
Do not alter the "on" events array
@jashkenas jashkenas merged commit dfec4b8 into jashkenas:master Mar 4, 2015
@jashkenas jashkenas added the fixed label Mar 4, 2015
jridgewell added a commit to jridgewell/backbone that referenced this pull request Mar 5, 2015
@jridgewell
Implement Events as a linked list.
630a335 
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
change fixed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jridgewell @akre54 @braddunbar @jashkenas