Remove hardcoded node_modules in gulpfile. #8890
Conversation
1. Use `npx` to execute binaries rather than hardcoded `node_modules/.bin`, which may not always be the right location. 2. Streamline generateDocumentation by calling it sync.
|
Thanks for the pull request @mramato!
Reviewers, don't forget to make sure that:
|
|
|
||
| //Run cloc on primary Source files only | ||
| var source = new Promise(function (resolve, reject) { | ||
| cmdLine = | ||
| "perl " + | ||
| clocPath + | ||
| "npx cloc" + |
There was a problem hiding this comment.
I guess npx is ok, because it should run the version in node_modules/.bin if it exists, and only install the package if it doesn't already exist. This is important because installing the package from npm could end up installing the wrong version.
But it really shouldn't be necessary to use npx in any case. Previously we were using node_modules/cloc/lib/cloc, which is not guaranteed to work. The cloc package can be hoisted somewhere else for various reasons. This is not true of node_modules/.bin, though. The .bins should always be present, as long as the package has actually been installed. So node_modules/.bin/cloc should be equivalent to npx cloc except that the former won't ever install cloc or execute some random thing with that name from your path.
And if we do need to run node_modules/clock/lib/cloc directly (rather than the thing in .bin), you can just ask node to resolve it first. Something like this (not tested):
const clocPath = path.resolve(path.dirname(require.resolve("cloc/package.json")), "cloc", "lib", "cloc");
There was a problem hiding this comment.
Kevin, the original problem that prompted this change was a literal node_modules/.bin/something in the gulpfile, which is only correct if you're not on Windows. This PR seems like a good opportunity to pick one consistent means of path construction and stick to it.
I never realized that npx will install missing packages -- kind of neat I guess? -- but if that's any kind of concern, there's a --no-install flag that you could use on all these invocations.
There was a problem hiding this comment.
@thw0rted Windows stores the binaries in node_modules/.bin, too. One potential gotcha, though, is that, on Windows, the programs in .bin are actually cmd scripts with a .cmd extension. So doing child_process.exec("node_modules/.bin/cloc", ...) won't work because there's no file called cloc and possibly also because cloc.cmd is not an executable. Long story short, add shell: true as an option to the exec call and you'll be good to go.
Is all this better than just using npx? Especially with --no-install? I guess not. After reading up on it some more, npx is better suited to this than I remembered. Carry on. :)
There was a problem hiding this comment.
I was referring to the path separator. node_modules/.bin/cloc does not work but I believe in my testing node_modules\\.bin\\.cloc worked. Anyway, I like how npx shows intent, handles nesting, global installs, etc. Ever since I discovered it a few years ago I've been trying to spread the word 😁
There was a problem hiding this comment.
Weird, Windows is usually ok with forward slashes. 🤷
|
@mramato LGTM -- I just ran |
Use
npxto execute binaries rather than hardcodednode_modules/.bin, which may not always be the right location.Streamline generateDocumentation by calling it sync.
@thw0rted when you get a chance can you verify this fixes the issues you were having? Should be same or close to the same as the changes you originally proposed.