-
-
Notifications
You must be signed in to change notification settings - Fork 32k
new core modules go under a namespace #21551
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
Changes from all commits
ebda268
70e0e5a
85b70c7
3be5486
0c2c0eb
51b01a0
ac230e7
00a45d3
124d8fb
b68f2fe
872e649
c2310c2
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 | ||||
---|---|---|---|---|---|---|
|
@@ -1234,6 +1234,12 @@ An invalid HTTP token was supplied. | |||||
|
||||||
An IP address is not valid. | ||||||
|
||||||
<a id="ERR_INVALID_NODE_BUILTIN"></a> | ||||||
### ERR_INVALID_NODE_BUILTIN | ||||||
|
||||||
An attempt was made to load a module from the Node.js namespace that doesn't | ||||||
exist. Only built in modules can be require from '@nodejs/'. | ||||||
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.
Suggested change
|
||||||
|
||||||
<a id="ERR_INVALID_OPT_VALUE"></a> | ||||||
### ERR_INVALID_OPT_VALUE | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,8 @@ const { | |
const { safeGetenv } = internalBinding('util'); | ||
const { | ||
makeRequireFunction, | ||
namespaceWhitelist, | ||
nonNamespaceBlacklist, | ||
requireDepth, | ||
stripBOM, | ||
stripShebang | ||
|
@@ -48,6 +50,7 @@ const experimentalModules = getOptionValue('--experimental-modules'); | |
|
||
const { | ||
ERR_INVALID_ARG_VALUE, | ||
ERR_INVALID_NODE_BUILTIN, | ||
ERR_REQUIRE_ESM | ||
} = require('internal/errors').codes; | ||
const { validateString } = require('internal/validators'); | ||
|
@@ -567,7 +570,16 @@ function tryModuleLoad(module, filename) { | |
} | ||
|
||
Module._resolveFilename = function(request, parent, isMain, options) { | ||
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. To the best of your knowledge, is there any popular module monkeypatching this? 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. This could make this a semver-major. As @mcollina suggests, we need to determine if this breaks anyone. Likely candidates are APM or testing libraries.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
Sorry, something went wrong. |
||
if (NativeModule.nonInternalExists(request)) { | ||
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. Re-post as this seems to be buried from the timeline: Maybe the correct place to insert this logic is It could also be faster if 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. Happy to update this, where would I find the source for NativeModule? |
||
if (request.startsWith('@nodejs/')) { | ||
request = request.slice(8); | ||
if (!namespaceWhitelist.has(request)) { | ||
throw new ERR_INVALID_NODE_BUILTIN(request); | ||
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. Can you please add a test to verify this case? I haven't found one in this PR. This is a very important feature that prevents misusage. |
||
} | ||
return request; | ||
} | ||
|
||
if (!nonNamespaceBlacklist.has(request) && | ||
NativeModule.nonInternalExists(request)) { | ||
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. Need to benchmark this change 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. Do you have a suggestion on how to do this or an exiting benchmark we can run? 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's possible a new benchmark would need to be written. We need to know the impact here |
||
return request; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
'use strict'; | ||
const { Worker } = require('worker_threads'); | ||
const { Worker } = require('@nodejs/worker_threads'); | ||
const path = require('path'); | ||
|
||
new Worker(path.resolve(__dirname, 'subprocess.js')); |
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.
I'm strongly -1 on renaming worker_threads at this point, fwiw