-
-
Notifications
You must be signed in to change notification settings - Fork 418
Don't report no-process-exit rule in worker threads
#388
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
Conversation
Rule no-process-exit should enable process.exit() when worker_threads module is required.
test/no-process-exit.js
Outdated
| 'const {workerData, parentPort} = require(\'worker_threads\')\n\nprocess.exit(1);', | ||
| 'import {workerData, parentPort} from \'worker_threads\'\n\nprocess.exit(1);', | ||
| 'import foo from \'worker_threads\'\n\nprocess.exit(1);', | ||
| '' |
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.
this empty code is introduced in #1 , but seems useleess
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.
I thought it was meant just to make sure the rule doesn't trigger by nothing. I just left it as is but I can remove it if needed.
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.
Done
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.
Actually, I'm not sure about it, maybe @sindresorhus need review this.
let's keep it unresolved
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.
Sure
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.
Makes sense to me to keep it. Just to be safe. We should really add it to all the rules.
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.
Ok. I'll restore it later today. Should I do it for others here or in a separate PR?
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.
Oh it got merged
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.
Doesn't matter. We should add it to all the rules.
fisker
left a comment
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.
LGTM
|
Just realized some commit came from another PR. Removed it and pushed again. |
no-process-exit rule in worker threads
Rule no-process-exit should enable process.exit()
when worker_threads module is required.
Fixes #328
IssueHunt Summary
Referenced issues
This pull request has been submitted to:
IssueHunt has been backed by the following sponsors. Become a sponsor