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

Change #off to #removeListener #266

Merged
merged 2 commits into from
Mar 27, 2017
Merged

Change #off to #removeListener #266

merged 2 commits into from
Mar 27, 2017

Conversation

paulkaplan
Copy link
Contributor

Fixes #256

.on is an alias for .addListener[1], but there is no alias for .off and it looks like there never will be [2] :( :( :(

[1] https://github.com/nodejs/node/blob/master/lib/events.js#L307
[2] nodejs/node#540

@paulkaplan paulkaplan requested a review from rschamp March 23, 2017 15:14
rschamp
rschamp suggested changes Mar 23, 2017
View reviewed changes
Copy link
Contributor

@rschamp rschamp left a comment

Choose a reason for hiding this comment

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

Since this is easy to miss, would you mind changing our .on()s to .addListener() so we aren't tempted to .off() elsewhere?

@rschamp rschamp removed their assignment Mar 23, 2017
@rschamp
Copy link
Contributor

rschamp commented Mar 23, 2017

Alternatively we could alias .off in the VM, but for now, ignoring that on exists is probably more bugproof.

@paulkaplan paulkaplan force-pushed the fix-listener-functions branch from 4bf7f3a to 7ad841d Compare March 27, 2017 14:07
@paulkaplan
Copy link
Contributor Author

@rschamp I changed all on and off to addListener and removeListener (respectively). Ready for re-review.

rschamp
rschamp approved these changes Mar 27, 2017
View reviewed changes
Copy link
Contributor

@rschamp rschamp left a comment

Choose a reason for hiding this comment

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

Thanks @paulkaplan!

@rschamp rschamp removed their assignment Mar 27, 2017
@paulkaplan paulkaplan merged commit a51d90b into scratchfoundation:develop Mar 27, 2017
@paulkaplan paulkaplan deleted the fix-listener-functions branch March 27, 2017 16:09
@ianstormtaylor ianstormtaylor mentioned this pull request Nov 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rschamp rschamp rschamp approved these changes

Assignees

@paulkaplan paulkaplan

Labels
pr - good to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulkaplan @rschamp