-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
util: fix isInsideNodeModules inside error #20266
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -356,16 +356,17 @@ function isInsideNodeModules() { | |
|
||
// Iterate over all stack frames and look for the first one not coming | ||
// from inside Node.js itself: | ||
for (const frame of stack) { | ||
const filename = frame.getFileName(); | ||
// If a filename does not start with / or contain \, | ||
// it's likely from Node.js core. | ||
if (!/^\/|\\/.test(filename)) | ||
continue; | ||
return kNodeModulesRE.test(filename); | ||
if (Array.isArray(stack)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this sufficient to fix the original bug with ts-node? That was triggering at original line 360 with frame existing but getFileName being undefined, which is inside the for loop. That seems to imply to me that it would not be fixed by an Array.isArray check. I'm not sure how to grab this to confirm that though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's because if stack is not an array then it's a string so it iterates on individual characters which obviously don't have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be good to throw in an extra check for the existence of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm... but we do want to minimize how much work we do here. There are two possibilities here:
There's no real alternative here. We can't end up calling a user-provided |
||
for (const frame of stack) { | ||
const filename = frame.getFileName(); | ||
// If a filename does not start with / or contain \, | ||
// it's likely from Node.js core. | ||
if (!/^\/|\\/.test(filename)) | ||
continue; | ||
return kNodeModulesRE.test(filename); | ||
} | ||
} | ||
|
||
return false; // This should be unreachable. | ||
return false; | ||
} | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
|
||
const bufferWarning = 'Buffer() is deprecated due to security and usability ' + | ||
'issues. Please use the Buffer.alloc(), ' + | ||
'Buffer.allocUnsafe(), or Buffer.from() methods instead.'; | ||
|
||
common.expectWarning('DeprecationWarning', bufferWarning, 'DEP0005'); | ||
|
||
// This is used to make sure that a warning is only emitted once even though | ||
// `new Buffer()` is called twice. | ||
process.on('warning', common.mustCall()); | ||
|
||
Error.prepareStackTrace = (err, trace) => new Buffer(10); | ||
|
||
new Error().stack; |
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.
Maybe just invert the conditional to make here?
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.
Yeah, I just wanted to keep the semantics exactly the same as before so
return false;
after everything, even if it's strictly-speaking unreachable from the for loop.