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: remove unnecessary assignment of exports #20143

Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 19, 2018

This commit removes the assignment of exports since it is not used
in these files and there is no harm re-assigning module.exports.

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

This commit removes the assignment of exports since it is not used
in these files and there is no harm re-assigning module.exports.
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. os Issues and PRs related to the os subsystem. labels Apr 19, 2018
@danbev
Copy link
Contributor Author

danbev commented Apr 19, 2018

@lpinca
Copy link
Member

lpinca commented Apr 19, 2018

there is no harm re-assigning module.exports

And it is reassigned anyway.

We are also documenting this pattern in https://nodejs.org/api/modules.html#modules_exports_shortcut. I honestly never liked it, I think it's confusing for newcomers.

@JacksonTian
Copy link
Contributor

I don't like this pattern too.

@trivikr
Copy link
Member

trivikr commented Apr 20, 2018

Re-run for CI node-test-commit-osx https://ci.nodejs.org/job/node-test-commit-osx/17983/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 22, 2018
@apapirovski
Copy link
Member

Landed in f31fc93

apapirovski pushed a commit that referenced this pull request Apr 22, 2018
This commit removes the assignment of exports since it is not used
in these files and there is no harm re-assigning module.exports.

PR-URL: #20143
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
This commit removes the assignment of exports since it is not used
in these files and there is no harm re-assigning module.exports.

PR-URL: #20143
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@danbev danbev deleted the remove-unnecessary-exports-assignment branch August 20, 2019 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants