Fix rimraf bin resolution #566
Conversation
src/FileSystemUtilities.js
Outdated
| if (err) return callback(err); | ||
| ChildProcessUtilities.spawn(join(npmBinPath.trim(), "rimraf"), [filePath], {}, callback); | ||
| }); | ||
| ChildProcessUtilities.spawn(require.resolve('rimraf/bin'), [filePath], {}, callback); |
There was a problem hiding this comment.
Linter's choking on the single-quotes here.
|
Nice! 👍 |
src/FileSystemUtilities.js
Outdated
| if (err) return callback(err); | ||
| ChildProcessUtilities.spawn(join(npmBinPath.trim(), "rimraf"), [filePath], {}, callback); | ||
| }); | ||
| ChildProcessUtilities.spawn(require.resolve("rimraf/bin"), [filePath], {}, callback); |
There was a problem hiding this comment.
This relies on the internal structure of the rimraf package not changing.
Could we combine this with a walk up to .bin so we can use the public interface?
Something like:
path.join(require.resolve("rimraf"), "../.bin/rimraf")There was a problem hiding this comment.
Yeah I thought about this but figured rimraf was pretty stable. Alternatively https://github.com/thlorenz/resolve-bin could be used, but I didn't want to include another dependency.
There was a problem hiding this comment.
To your idea, is ./.bin/rimraf guaranteed to be set in the same node_modules folder where ./rimraf/ is installed? I was worried this could differ across npm versions or yarn or something. But if that's not a problem, then yeah I like your idea. Would we need to use path.resolve in that case?
path.resolve(require.resolve("rimraf"), "../.bin/rimraf")
There was a problem hiding this comment.
I'm using https://github.com/thlorenz/resolve-bin in multiple projects to get proper path to bin file.
There was a problem hiding this comment.
Yeah, I guess relying on the relative location of the .bin/ directory isn't much different from relying on the location of bin.js in rimraf. 😆
I'd like to avoid adding a dep if possible.
Maybe a comment to the effect of "this will break if the location of the executable changes within rimraf" would be enough to help us fix things up quickly in the unlikely case that it actually moves.
There was a problem hiding this comment.
👍 sounds good. I'll add a comment.
|
LGTM. Thanks @rtsao! |
|
awesome |
|
I think it affected us too (https://travis-ci.org/ckeditor/ckeditor5/builds/197615503#L1201) so we're waiting for a new release :) |
|
Ok released beta.36! |
|
This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
On NPM3,
rimrafis being hoisted to the parent node_modules, soexec("npm bin", {cwd: __dirname},produces the wrong bin path. So in the current version of lerna, the clean command is broken.I've simplified the logic to use
require.resolve, which will work regardless of where rimraf is. This should also be faster as well!Fixes bug introduced in: #547