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: improve isInsideNodeModules #52147
base: main
Are you sure you want to change the base?
Conversation
d08b72a
to
0778337
Compare
Just curious, did you do that for performance reasons, or is that a neutral change to make the regex more intention-revealing? |
I just saw that the capturing group is not used and the captured string can be very long. So i had performance in mind, but i didnt wanted to claim a (significant?) performance gain, which i probably cant prove. |
Looks like this a breaking the following tests:
|
I will have a look at it in few hours ;) |
0778337
to
c714cda
Compare
Should be fixed now. |
@nodejs/node-core-utils can this be merged? |
if (RegExpPrototypeExec(/^\/|\\/, filename) === null) | ||
if ( | ||
filename[0] !== '/' && | ||
StringPrototypeIncludes(filename, '\\') === false |
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.
This only applies to Windows right?
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.
this PR is already old but afaik it was effecting linux too
Improve isInsideNodeModules by avoiding a Regex
kNodeModulesRE is using a non-capturing group