Skip to content

lib: make the builtInObjects set deterministic during bootstrap #33049

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.

Sign up for GitHub

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

Closed
joyeecheung wants to merge 2 commits into from
Closed

lib: make the builtInObjects set deterministic during bootstrap #33049

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bedf4d
lib: make the builtInObjects set deterministic during bootstrap
joyeecheung Apr 24, 2020
5f01a4f
fixup! lib: make the builtInObjects set deterministic during bootstrap
joyeecheung Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
fixup! lib: make the builtInObjects set deterministic during bootstrap
  • Loading branch information
@joyeecheung
joyeecheung committed Apr 30, 2020
commit 5f01a4f52e1cda83c19864b0c6cbfc00b120fc83
2 changes: 1 addition & 1 deletion lib/internal/bootstrap/pre_execution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ function setupDebugEnv() {
}

const { builtInObjects } = require('internal/util/inspect');
// The content of this list depends on e.g. v8 flags, as well as
// 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)) {

@joyeecheung joyeecheung Apr 24, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@BridgeAR BridgeAR May 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];
}

builtInObjects.add(key);
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/util/inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,11 @@ let hexSlice;

// 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
// a) Whether 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
// b) Whether this file is required before or after bootstrap
// (Node.js adds things like Buffer, URL, etc.)
const builtInObjects = new Set([
'Object',
Expand Down