lib: make the builtInObjects set deterministic during bootstrap #33049
Conversation
Make this list explicit instead of filtering over globalThis so that this list is deterministic during the bootstrap regardless of: a) Wether this file is required when the context is going to be serialized, or what the V8 flags are. V8 does not add SharedArrayBuffer, Atomics, WebAssembly, and other flaggable globals e.g. harmony ones to a context that will be serialized. b) Wether this file is required before or after bootstrap (Node.js adds things like Buffer, URL, etc.)
nodejs-github-bot
added
the
util
|
cc @BridgeAR @addaleax (because I found this when working on #32761 (comment)) |
| const { builtInObjects } = require('internal/util/inspect'); | ||
| // The content of this list depends on e.g. v8 flags, as well as | ||
| // what gets added by Node.js to the global object. | ||
| for (const key of ObjectGetOwnPropertyNames(globalThis)) { |
There was a problem hiding this comment.
This now contains these added by Node.js
'global',
'process',
'Buffer',
'URL',
'URLSearchParams',
'TextEncoder',
'TextDecoder',
'clearInterval',
'clearTimeout',
'setInterval',
'setTimeout',
'queueMicrotask',
'clearImmediate',
'setImmediate'
We still need this loop if we want to pick up what get's added by V8 through harmony flags (like WeakRefs). I wonder which of these do we want to skip? @BridgeAR
There was a problem hiding this comment.
We mostly do not need those. The only ones that we should keep are global classes that have a prototype. The current RegExp is not matching exactly what we actually need. I just checked and this seems to ideally model what is needed for inspect:
function isConstructor(name) {
try {
Reflect.construct(String, [], globalThis[name]);
} catch {
return false;
}
return globalThis[name].prototype !== undefined && name[0].toUpperCase() === name[0];
}
|
Blocking this for now so that this set serves as a smoke test for a reland of https://chromium-review.googlesource.com/c/v8/v8/+/2143983 |
| 'Infinity', | ||
| 'NaN', |
There was a problem hiding this comment.
| 'Infinity', | |
| 'NaN', |
| 'NaN', | ||
| 'Boolean', | ||
| 'String', | ||
| 'Symbol', |
| 'JSON', | ||
| 'Math', | ||
| 'Intl', |
There was a problem hiding this comment.
| 'JSON', | |
| 'Math', | |
| 'Intl', |
| 'Proxy', | ||
| 'Reflect' |
There was a problem hiding this comment.
| 'Proxy', | |
| 'Reflect' | |
| 'SharedArrayBuffer' |
| const { builtInObjects } = require('internal/util/inspect'); | ||
| // The content of this list depends on e.g. v8 flags, as well as | ||
| // what gets added by Node.js to the global object. | ||
| for (const key of ObjectGetOwnPropertyNames(globalThis)) { |
There was a problem hiding this comment.
We mostly do not need those. The only ones that we should keep are global classes that have a prototype. The current RegExp is not matching exactly what we actually need. I just checked and this seems to ideally model what is needed for inspect:
function isConstructor(name) {
try {
Reflect.construct(String, [], globalThis[name]);
} catch {
return false;
}
return globalThis[name].prototype !== undefined && name[0].toUpperCase() === name[0];
}
BridgeAR
force-pushed
the
master
branch
2 times, most recently
from
8ae28ff
to
2935f72
Compare
|
@joyeecheung do you still work on this? |
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
Make this list explicit instead of filtering over globalThis so that
this list is deterministic during the bootstrap regardless of:
a) Wether this file is required when the context is going to be
serialized, or what the V8 flags are. V8 does not add
SharedArrayBuffer, Atomics, WebAssembly, and other flaggable
globals e.g. harmony ones to a context that will be serialized.
b) Wether this file is required before or after bootstrap
(Node.js adds things like Buffer, URL, etc.)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes