-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore: Refactor library to use smaller modules #91
Conversation
The `index.js` file got pretty big over time. This commit splits it into smaller modules: - The core logic remains in `index.js` - The `munge` functions are moved to the `lib/munge` directory. Each module detects and wraps one kind of target. - Helper functions are moved out of `index.js` to the `lib` directory Using multiple modules instead of a single `index.js` enables easier future extensions.
const osHomedir = require('os-homedir') | ||
|
||
const home = process.env.SPAWN_WRAP_SHIM_ROOT || osHomedir() | ||
const homedir = home + '/.node-spawn-wrap-' |
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.
path.resolve?
} | ||
|
||
let npmPath = whichOrUndefined('npm') || 'npm' | ||
npmPath = path.dirname(npmPath) + '\\node_modules\\npm\\bin\\npm-cli.js' |
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.
Would love to eliminate path concatenation, use path.join instead. Again this can be a follow-up, I know what's here is just a straight move from index.js.
} | ||
|
||
if (hasMain) { | ||
const replace = workingDir + '/' + command |
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.
follow-up: path.join
options.originalNode = shebangbin | ||
options.basename = maybeNode | ||
options.file = shebangbin | ||
options.args = [shebangbin, workingDir + '/' + maybeNode] |
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.
follow-up: path.join
Oh @demurgos if you make these changes please just add a commit, I will squash when I do the merge. This will make it easier for me to re-review. |
Thanks for the reviews. I'll address the comments tomorrow (moving let, fixing Edit: I'm not sure if I'll have time this week-end, but I'll try to address your comments. |
force-pushing is fine for the smaller PR's. |
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.
Adding some notes on top of Corey's review. Thanks for this!
if (!IS_DEBUG) { | ||
return; | ||
} | ||
const prefix = 'SW ' + process.pid + ': ' |
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.
We should use template strings
This commit inlines the `KNOWN_SHELLS` constant inside `isSh` and uses a simple array instead of a set.
In debug mode, the shim directory is not cleaned-up but the `signalExit` handler is still passed. This commit simplifies the code by avoiding to attach the handler altogether when it has no effect.
I fixed most of the comments relative to minor issues. I'll leave replacing string concatenation with dedicated functions to a follow-up PR because it is an important topic. I think that this PR can be merged as-is. You can squash the commits or simply rebase (each one is a self-contained minor fix) |
For semantic versioning I think this will need to be squashed into the chore commit. But thanks @demurgos! I'll take a closer look soon! |
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.
👍 nice cleanup;
Be very cautious when rolling this out, just because of how finicky the module is; probably we should go to a next
branch, and test on a wider variety of libraries than we usually do before a release.
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'll leave the merge and release to @bcoe or @coreyfarrell
The
index.js
file got pretty big over time. This commit splits it into smaller modules:index.js
munge
functions are moved to thelib/munge
directory. Each module detects and wraps one kind of target.index.js
to thelib
directoryUsing multiple modules instead of a single
index.js
enables easier future extensions.