-
Notifications
You must be signed in to change notification settings - Fork 24
fix: autolocation of node-gyp in make-spawn-args #221
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
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.resolveis not available.npm_config_node_gypwhen node-gyp not resolved successfully.Related
npm_config_node_gypenvironment variable andnpm config set node-gypconfig don't work with npm 7 #23node-gyp" not respected in npm 7 cli#2839