-
Notifications
You must be signed in to change notification settings - Fork 30k
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
stream: Readable.off #29486
stream: Readable.off #29486
Conversation
We have special logic in removeListener() which must apply to off() as well.
perhaps we should change ee.off to call this.removeListener instead of being an alias? this seems surprising for anyone extending ee. |
I would prefer @devsnek's suggestion but I am still LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Alternative: #29503. |
@benjamingr I’m fine with either. |
I don't think that makes any more sense than this PR. Would make sense for EE to ensure this doesn't happen at all. However, as you point out it looks and is quite scary. I think this PR is the most appropriate right now. |
I was recommending what #29503 does, I see having to manually override this as a bug in EventEmitter's API. |
I think this is semver-patch while #29503 is semver-major. I still think it makes sense to merge this first? |
I think we should land this and backport it. |
@Trott: This looks ready? |
We have special logic in removeListener() which must apply to off() as well. PR-URL: #29486 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Landed in 1665a93 |
We have special logic in removeListener() which must apply to off() as well. PR-URL: #29486 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
We have special logic in removeListener() which must apply to off() as well. PR-URL: #29486 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
We have special logic in removeListener() which must apply
to off() as well.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes