-
Notifications
You must be signed in to change notification settings - Fork 24
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
fix: autolocation of node-gyp in make-spawn-args #221
Conversation
18186ac
to
ceef07a
Compare
let npm_config_node_gyp | ||
try { | ||
/* istanbul ignore next */ | ||
if (typeof require === 'function' && typeof require.resolve === 'function') { |
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.
The typeof require
may seem redundant here considering we call require('path')
just prior.
Rightfully or not, bundlers like webpack and esbuild may transform the direct require()
calls but leave require.resolve
intact. The extra check prevents runtime errors in such scenarios.
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 think for now we need the runtime error here. If this isn't working then this module shouldn't think it is working.
run-script/lib/node-gyp-bin/node-gyp
Line 2 in a8b4034
node "$npm_config_node_gyp" "$@" |
ceef07a
to
2780f78
Compare
prevents runtime error in environments where `require.resolve` is not available Fall back to any existing `npm_config_node_gyp` provided by environment
2780f78
to
c936a18
Compare
@reggi @hashtagchris Some attention on this and #222 would be much appreciated if you are able to 🙏 |
This seems like it is a breaking change and I think we should not land this right now. Now that Let's land npm/cli#8129, then I can update #222 to gracefully handle that param, and we can go from there. |
require.resolve
is not available.npm_config_node_gyp
when node-gyp not resolved successfully.Related
npm_config_node_gyp
environment variable andnpm config set node-gyp
config don't work with npm 7 #23node-gyp
" not respected in npm 7 cli#2839