Prevent require-ing bin/shjs #848
Conversation
bin/shjs
Outdated
| console.log(); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (require.main !== module) { |
There was a problem hiding this comment.
nit: can you move this up? Also, I assume you moved require('../global') down so that it doesn't pollute the test--this is OK, but you need to add a comment explaining why.
bin/shjs
Outdated
|
|
||
| if (process.argv.length < 3) { | ||
| console.log('ShellJS: missing argument (script name)'); | ||
| function exit(msg) { |
There was a problem hiding this comment.
nit: rename this function (it's too easy to confuse this with shell.exit()). Suggestion: exitWithErrorMessage()
|
@nfischer PTAL |
Codecov Report
@@ Coverage Diff @@
## master #848 +/- ##
======================================
Coverage 95.8% 95.8%
======================================
Files 34 34
Lines 1262 1262
======================================
Hits 1209 1209
Misses 53 53Continue to review full report at Codecov.
|
bin/shjs
Outdated
| exitWithErrorMessage('ShellJS: missing argument (script name)'); | ||
| } | ||
|
|
||
| // we only need ShellJS methods past this point, so require here |
There was a problem hiding this comment.
Our usual style is to import at the top, not to import immediately before use. If there's a real requirement that we import down here, please document that requirement.
I suspect the requirement is: "we must import this after the require.main check, otherwise we pollute the global namespace as a side effect of the disallow require-ing test case."
If that's the real requirement, then you should move this immediately below the require.main check (and indicate this requirement). Otherwise, please explain why this is the appropriate spot, because I can't figure that out.
There was a problem hiding this comment.
Correct, the require at least needs to be after the require.main check, because the error it throws is catchable, and we don't want any side effects, as you said. I also moved it after the process.argv check, because we still don't need any globals at that point, but the fail condition is exiting. There is less of a benefit to require-ing after this check, so I will move it up.
Fixes #789
This makes it so that
bin/shjscannot berequired as a module, and adds a test for this.