feat: support 'node:'-prefixed core modules #53
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since v16.0.0, v14.18.0 node has supported a 'node:'-prefix on require() of core modules. They typically are an alias for unprefixed builtin modules, e.g. 'http' and 'node:http' yield the same module. However, new core modules may be added only with the prefix, e.g. 'node:test' added in node v18.0.0. https://nodejs.org/api/modules.html#core-modules This change adds support for a require-in-the-middle hook to capture the node-prefixed aliases. I.e. `Hook(['http', ...], onRequire)` will hook into `require('node:http')` and `require('http')`. Refs: elastic/apm-agent-nodejs#2816
|
I'll re-request review once I get tests passing. |
|
Can't actually have a CI test with node v6.0 because the Full npm install error |
trentm
commented
Aug 10, 2022
| isCore = moduleName => { | ||
| // Prefer `resolve.core` lookup to `resolve.isCore(moduleName)` because the | ||
| // latter is doing version range matches for every call. | ||
| return !!resolve.core[moduleName] |
There was a problem hiding this comment.
REVIEW NOTE: The resolve module has a core property that looks like this:
> process.version
'v16.16.0'
> resolve.core
{
assert: true,
'node:assert': true,
'assert/strict': true,
'node:assert/strict': true,
async_hooks: true,
'node:async_hooks': true,
...
'node:test': false,
...
Those true/false values are resolved from node version ranges against the current process.version. For example node:test is not a core module in node v16.16.0 that I was running this with. This makes resolve.core a fast way to look up if a given module id is core for the current node version. This obviates the need for #42. (@harttle sorry that your PR for this never got merged.) It is also available back to the supported node v6, so we don't need the filename.includes(path.sep) fallback.
From https://nodejs.org/api/modules.html#core-modules
The list of core modules that can be loaded without using the node: prefix is exposed as module.builtinModules.
In node v18, module.builtinModules does not include node:test. So, our replacing usage of module.builtinModules handles this case as well.
Merged
1 task
astorm
approved these changes
Aug 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since v16.0.0, v14.18.0 node has supported a 'node:'-prefix on
require() of core modules. They typically are an alias for unprefixed
builtin modules, e.g. 'http' and 'node:http' yield the same module.
However, new core modules may be added only with the prefix, e.g.
'node:test' added in node v18.0.0.
https://nodejs.org/api/modules.html#core-modules
This change adds support for a require-in-the-middle hook to capture
the node-prefixed aliases. I.e.
Hook(['http', ...], onRequire)willhook into
require('node:http')andrequire('http').Refs: elastic/apm-agent-nodejs#2816
@astorm Would you like to review this? I'll get a draft PR for elastic/apm-agent-nodejs#2816 as well that includes a test case of instrumenting
require('node:http')once we have a release of this the APM agent can upgrade to.