-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
feat(jest-resolve): detect node:
-prefixed built-in modules
#11686
feat(jest-resolve): detect node:
-prefixed built-in modules
#11686
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11686 +/- ##
=======================================
Coverage 69.01% 69.01%
=======================================
Files 312 312
Lines 16335 16336 +1
Branches 4734 4735 +1
=======================================
+ Hits 11273 11274 +1
Misses 5034 5034
Partials 28 28
Continue to review full report at Codecov.
|
Could you rerun |
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.
Hey! 👋
There is already an open PR to fix that: #11638
And there is an open issue too: #11637
Concerning the implementation itself, I think that isBuiltinModule
should returns true
or false
if the actual module name is a builtin module, whatever the path is.
Whereas isCoreModule
also take care to check the path so, we should probably fix this in the ìsCoreModule
function I edited in my PR.
I might be mistaken but it is what I think.
Also you forgot to update CHANGELOG.md
. 😄
@@ -23,4 +23,8 @@ describe('isBuiltinModule', () => { | |||
it('should return false for any internal node builtins', () => { | |||
expect(isBuiltinModule('internal/http')).toBe(false); | |||
}); | |||
|
|||
it('should return true for the `node:http` module', () => { |
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.
We should probably also test that node:not-a-code-module
returns 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.
Reasonable. I'll add this case, thanks.
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.
Hmm... isBuiltinModule
lays deeper than isCoreModule
, and imho the fix should be applied as much closer to the point where the issue raises. Anyway, I'd be happy if any of our PRs would have merged as soon as possible.
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.
Anyway, I'd be happy if any of our PRs would have merged as soon as possible.
Yes, hopefully, one of our PR will get merged soon.
// Core modules can also be identified using the node: prefix, for instance, require('node:http') | ||
|
||
return BUILTIN_MODULES.has( | ||
module.startsWith('node:') ? module.slice(5) : module, |
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.
Here we can avoid the ternary operator, and do it with Regex:
export default function isBuiltinModule(module: string): boolean {
module = module.replace(/^node:/, '');
return BUILTIN_MODULES.has(module);
}
See discussion: #11638 (comment)
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.
Hey @divlo,
- Just thought, that regex might be slower.
- Argument mutation is not "the best practice".
var Benchmark = require('benchmark')
var suite = new Benchmark.Suite;
const moduleName = 'node:foobar'
suite
.add('String#replace', function() {
moduleName.replace(/^node:/, '');
})
.add('String#startsWith', function() {
moduleName.startsWith('node:') ? moduleName.slice(5) : moduleName;
})
.on('cycle', function(event) {
console.log(String(event.target));
})
.on('complete', function() {
console.log('Fastest is ' + this.filter('fastest').map('name'));
})
.run({ 'async': true });
node bench.js
String#replace x 17,913,326 ops/sec ±1.19% (87 runs sampled)
String#startsWith x 57,254,145 ops/sec ±0.73% (88 runs sampled)
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.
Indeed! 👍
I also ran it on my laptop :
String#replace x 25,474,843 ops/sec ±0.37% (96 runs sampled)
String#startsWith x 86,410,568 ops/sec ±0.21% (98 runs sampled)
Fastest is String#startsWith
Great one, will also change this in my PR.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Problem
Summary
https://nodejs.org/api/modules.html#modules_core_modules
Fix
Test plan
jest /Users/a.golub/projects/gh/jest/packages/jest-resolve/src/__tests__/isBuiltinModule.test.ts