Backport removing let in for loop declaration #9553
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
lib / src
This is a known de-opt. It may not be 100% necessary in all cases but it seems like a decent enough idea to avoid it. Ref: nodejs#9553 PR-URL: nodejs#8873 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Since 2e568d9 there is a bug where unpiping a stream from a readable stream that has `_readableState.pipesCount > 1` will cause it to remove the first stream in the `_.readableState.pipes` array no matter where in the list the `dest` stream was. This patch corrects that problem. Ref: nodejs#9553 PR-URL: nodejs#9171 Fixes: nodejs#9170 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Since 2e568d9 there is a bug where unpiping a stream from a readable stream that has `_readableState.pipesCount > 1` will cause it to remove the first stream in the `_.readableState.pipes` array no matter where in the list the `dest` stream was. Ref: nodejs#9553 PR-URL: nodejs#9171 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#9553 Ref: nodejs#8873 PR-URL: nodejs#9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins
force-pushed
the
no-let-in-loops
branch
from
7b2d24f
to
d1d3379
Compare
MylesBorins
added
stream
MylesBorins
changed the title
MylesBorins
pushed a commit
that referenced
this pull request
Nov 14, 2016
This is a known de-opt. It may not be 100% necessary in all cases but it seems like a decent enough idea to avoid it. Ref: #9553 PR-URL: #8873 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Nov 14, 2016
Since 2e568d9 there is a bug where unpiping a stream from a readable stream that has `_readableState.pipesCount > 1` will cause it to remove the first stream in the `_.readableState.pipes` array no matter where in the list the `dest` stream was. This patch corrects that problem. Ref: #9553 PR-URL: #9171 Fixes: #9170 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Nov 14, 2016
Since 2e568d9 there is a bug where unpiping a stream from a readable stream that has `_readableState.pipesCount > 1` will cause it to remove the first stream in the `_.readableState.pipes` array no matter where in the list the `dest` stream was. Ref: #9553 PR-URL: #9171 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Nov 14, 2016
This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: #9045 Ref: #9553 Ref: #8873 PR-URL: #9049 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
landed in 84849f1...c8dccf2 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
lib, tools, test, streams
Description of change
This is a backport of a number of commits that remove the use of
letin for loop declarations.It starts with #8873 that removes all instances of let, followed by #9171 to fix a regression introduced in streams, and finally #9049 which adds a linting rule to avoid regressing.
These changes have been living on v6.x and v7.x for a while and did show a decent improvement in performance. I would imagine it will only be more noticeable for v4.x. If anyone from @nodejs/benchmarking wants to chime in here that would be awesome