-
Notifications
You must be signed in to change notification settings - Fork 29.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
module: use kNodeModulesRE to detect node_modules #55243
Conversation
This is faster and more consistent with other places using the regular expression to detect node_modules.
Review requested:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55243 +/- ##
==========================================
- Coverage 88.39% 88.39% -0.01%
==========================================
Files 652 652
Lines 186568 186563 -5
Branches 36047 36043 -4
==========================================
- Hits 164924 164916 -8
+ Misses 14915 14912 -3
- Partials 6729 6735 +6
|
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.
Can you share some benchmarks?
Personally I am not a fan of adding microbenchmarks of internals into the benchmark suite, so I'll just share some local benchmark results: const path = require('path');
const fs = require('fs');
const kNodeModulesRE = /^(?:.*)[\\/]node_modules[\\/]/;
function isUnderNodeModules(filename) {
return filename && kNodeModulesRE.exec(filename) !== null;
}
function isUnderNodeModulesByPath(filename) {
const resolvedPath = path.resolve(filename);
const normalizedPath = path.normalize(resolvedPath);
const splitPath = normalizedPath.split(path.sep);
return splitPath.includes('node_modules');
}
const check = process.env.TYPE === 'PATH' ? isUnderNodeModulesByPath : isUnderNodeModules;
const files = fs.globSync('**/*.js');
const results = [];
console.time(files.length);
for (let i = 0; i < files.length; ++i) {
results.push(check(files[i]));
}
console.timeEnd(results.length);
|
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.
LGTM I remember I created that function as a workaround, thanks for handling it properly
Landed in bdd590b |
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Backport-PR-URL: nodejs#55217 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Refs: nodejs#52697
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Backport-PR-URL: nodejs#55217 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Refs: nodejs#52697
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Backport-PR-URL: nodejs#55217 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Refs: nodejs#52697
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Backport-PR-URL: nodejs#55217 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Refs: nodejs#52697
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
This is faster and more consistent with other places using the regular expression to detect node_modules. PR-URL: nodejs#55243 Backport-PR-URL: nodejs#55217 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Refs: nodejs#52697
This is faster and more consistent with other places using the regular expression to detect node_modules.