-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Update wrong catch block #2441
Update wrong catch block #2441
Conversation
Yeah, sorry, this is intentional (IIRC), there's some OS' where this fails, I can't remember the details, it was some trimmed-down Linux, I'm sure Is this causing problems for you? |
hmm I had a few other issues that were causing me to get to this point anyway, but this is disguising the error, as it gives me
but not a big problem :) |
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.
ookkaayy ... my memory is failing me on this one, I see this particular change was introduced @ fca4795 and that I must be thinking of another silent catch that we introduced eons ago.. So this is pretty new, and a legit parse error.
👍 so let's fix the parse error you're getting, but not force the throw.
ahha! I knew I wasn't going that senile: #1835 but it's in install.js, while this same code is now in find-python.js which was just recently refactored. |
Co-authored-by: Rod Vagg <rod@vagg.org>
ah alright! nice, so we can at least fix the unexpected token thing |
Why I used
|
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.
👍 Avoids a syntax error on older Node (Node 9.x or older).
(Note: Those versions of Node are technically not supported by the latest node-gyp
, because of #2123 --> #2152. But I think not causing a syntax error in the program for older Node is still an improvement.)
Due to #2152, users should really be on Node 10.12 or newer when using the latest node-gyp
, and ideally they would be on the latest Node 12.x or 14.x or 16.x. But I don't think fixing this syntax error for older Node (at the cost of three characters of JS) hurts anything.
👍 thanks @DeeDeeG, it's good to lean on the conservative side for node-gyp since it's in such wide usage. When people show up using older versions of Node.js (for whatever reason - sometimes they can't change) and well tell them that their old version of node-gyp doesn't work for their use-case, it's not fun to tell them they can't get the newer one cause their Node.js is old. So where the change is painless, like this one, let's opt for the conservative choice. |
Update: I found out that trying to use Here's the details and a rather hacky workaround... #2220 broke Workaround if you really need to run the latest
|
Checklist
npm install && npm test
passesDescription of change