-
Notifications
You must be signed in to change notification settings - Fork 231
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
replace bind() #253
replace bind() #253
Conversation
599c750
to
f740369
Compare
f740369
to
d0225d1
Compare
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.
Amazing, thanks!
LGTM
ok this works fine because we put it through babel to deal with the arrow functions, but I wonder why node core couldn't use the arrow function? |
|
@mcollina s/arrow functions/inline closures/ |
@mscdex yes sorry. |
so readable stream is intended for use in all versions of node/multiple browsers so I'm not sure the wisdom of including optimizations for older platforms, chrome for instance will be negatively effected by this |
@calvinmetcalf Well, it's not just older node versions but also possibly other javascript engines that might not optimize the same things. I personally do not see a straight-forward solution because I just created these PRs because @mcollina had mentioned in the upstream node PR that he considered performance changes to be breaking. |
@calvinmetcalf using |
We are going to merge this on the next update, as decided in #252 |
Part of #262 |
This is anticipation of Writable changes landing in nodejs/node#6533
/cc @calvinmetcalf @mcollina