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.

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

lib: use destructuring for constants on the top-level module scope #16063

Closed

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Oct 7, 2017

This change is to unify the declaration for constants into using destructuring on the top-level-module scope, reducing some redundant code.

By the way, what about adding "prefer-destructuring" into the .eslintrc ?

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

lib

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 7, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, but be prepared to rebase if it becomes necessary :)

@BridgeAR
Copy link
Member

BridgeAR commented Oct 9, 2017

The commit message exceeds the 50 character maximum though. It would be nice if this could be fixed before landing.

@starkwang starkwang force-pushed the use-destruct-for-constants branch from 993bbee to 49f469e Compare October 9, 2017 09:46
@starkwang
Copy link
Contributor Author

@BridgeAR I've just shortened the commit message : )

@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2017

By the way, what about adding "prefer-destructuring" into the .eslintrc ?

Seems reasonable to me.

parsers,
freeParser,
debug
} = common;
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you can add _checkIsHttpToken: checkIsHttpToken too here, which would get rid of the only reference to common in the file other than this one. Also would you mind sorting these in alphabetical order?

CRLF,
continueExpression,
chunkExpression,
httpSocketSetup
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, but with _checkInvalidHeaderChar instead.

const {
_validateStdio,
setupChannel
} = child_process;
const ChildProcess = exports.ChildProcess = child_process.ChildProcess;
Copy link
Member

Choose a reason for hiding this comment

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

This can be refactored as well.

@starkwang starkwang force-pushed the use-destruct-for-constants branch from 49f469e to 6c53ee8 Compare October 11, 2017 04:54
@starkwang
Copy link
Contributor Author

Pushed commit to address comments.

This change is to unify the declaration for constants into using
destructuring on the top-level-module scope, reducing some redundant
code.
@starkwang starkwang force-pushed the use-destruct-for-constants branch from 6c53ee8 to 4fa9357 Compare October 11, 2017 04:58
@tniessen
Copy link
Member

@tniessen tniessen self-assigned this Oct 16, 2017
@tniessen
Copy link
Member

Landed in 212de3c, thanks! 😃

@tniessen tniessen closed this Oct 16, 2017
tniessen pushed a commit that referenced this pull request Oct 16, 2017
This change is to unify the declaration for constants into using
destructuring on the top-level-module scope, reducing some redundant
code.

PR-URL: #16063
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
This change is to unify the declaration for constants into using
destructuring on the top-level-module scope, reducing some redundant
code.

PR-URL: nodejs/node#16063
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins
Copy link
Contributor

Does this change offer any performance improvements? These kind of large changes are going to create backporting conflicts that will be hard to manage for LTS

With that being said this does not land cleanly on 8.x. Would someone be willing to manually backport

@lance
Copy link
Member

lance commented Oct 25, 2017

@MylesBorins just submitted backport PR: #16494. The main conflicts that arose had to do with the semver-major commits around internal/errors like this one. Just FYI.

gibfahn pushed a commit that referenced this pull request Oct 30, 2017
This change is to unify the declaration for constants into using
destructuring on the top-level-module scope, reducing some redundant
code.

PR-URL: #16063
Backport-PR-URL: #16494
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
This change is to unify the declaration for constants into using
destructuring on the top-level-module scope, reducing some redundant
code.

PR-URL: #16063
Backport-PR-URL: #16494
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 16, 2017

I'm not landing this on v6.x

But want to once again urge collaborators that would should consider holding off on larger changes like this until v6.x goes into maintenance, this has the potential to cause a bunch of conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.