-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
vm: introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY #54394
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
Conversation
2f4dbe9 to
3e87824
Compare
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
This comment was marked as outdated.
This comment was marked as outdated.
3e87824 to
055c9c9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54394 +/- ##
=======================================
Coverage 87.32% 87.33%
=======================================
Files 649 649
Lines 182603 182590 -13
Branches 35042 35039 -3
=======================================
+ Hits 159464 159470 +6
+ Misses 16400 16391 -9
+ Partials 6739 6729 -10
|
94e06f3 to
8241574
Compare
legendecas
left a 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.
Overall LGTM. Thanks!
| if (!ctor_name->Equals(v8_context, env->object_string()) | ||
| .FromMaybe(false) && | ||
| new_context_global | ||
| ->DefineOwnProperty( |
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.
Not really relevant to this PR but I think we should not automatically create an own @@ToStringTag property on the global object: #54522.
This implements a flavor of vm.createContext() and friends
that creates a context without contextifying its global object.
This is suitable when users want to freeze the context (impossible
when the global is contextified i.e. has interceptors installed)
or speed up the global access if they don't need the interceptor
behavior.
```js
const vm = require('node:vm');
const context = vm.createContext(vm.constants.DONT_CONTEXTIFY);
// In contexts with contextified global objects, this is false.
// In vanilla contexts this is true.
console.log(vm.runInContext('globalThis', context) === context);
// In contexts with contextified global objects, this would throw,
// but in vanilla contexts freezing the global object works.
vm.runInContext('Object.freeze(globalThis);', context);
// In contexts with contextified global objects, freezing throws
// and won't be effective. In vanilla contexts, freezing works
// and prevents scripts from accidentally leaking globals.
try {
vm.runInContext('globalThis.foo = 1; foo;', context);
} catch(e) {
console.log(e); // Uncaught ReferenceError: foo is not defined
}
console.log(context.Array); // [Function: Array]
```
vm: introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY
Text for releasers:
new option for vm.createContext() to create a context with a freezable globalThis
This implements a flavor of
vm.createContext()and friends that creates a context without contextifying its global object when vm.constants.DONT_CONTEXTIFY is used.This is suitable when users want to freeze the context (impossible when the global is contextified i.e. has interceptors installed) or speed up the global access if they don't need the interceptor behavior.