Backport removing let in for loop declaration#9553
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or 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 <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
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#9171Fixes: nodejs#9170 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Myles Borins <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Myles Borins <[email protected]>
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 <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorinsforce-pushed the no-let-in-loops branch from 7b2d24f to d1d3379CompareMylesBorins added stream MylesBorins changed the title ContributorAuthor
MylesBorins commented Nov 11, 2016
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 <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
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: #9171Fixes: #9170 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Myles Borins <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Myles Borins <[email protected]>
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 <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]>
ContributorAuthor
MylesBorins commented Nov 14, 2016
landed in 84849f1...c8dccf2 |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
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