Skip to content
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

vm: introduce vm.Context #30709

Closed
wants to merge 2 commits into from
Closed

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Nov 28, 2019

Fixes #855
Fixes #31658
Fixes #31808

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@devsnek devsnek added the vm Issues and PRs related to the vm subsystem. label Nov 28, 2019
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 28, 2019
@devsnek devsnek added wip Issues and PRs that are still a work in progress. and removed c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 28, 2019
@devsnek
Copy link
Member Author

devsnek commented Nov 28, 2019

the vm.Context constructor could also have a globalProxy option which takes an object. Should it?

@devsnek

This comment has been minimized.

@devsnek devsnek force-pushed the new-vm-context branch 3 times, most recently from f69a810 to 55e15ff Compare December 4, 2019 18:25
@devsnek
Copy link
Member Author

devsnek commented Dec 5, 2019

@nodejs/vm the vm.Context constructor could also have a globalProxy option which takes an object, like createContext(globalProxy), except it doesn't do anything to the object. Should it?

@joyeecheung
Copy link
Member

joyeecheung commented Dec 10, 2019

What's the reasoning behind deprecating vm.createContext()? It has been around since v0.3.1 and I am pretty sure it would require significant effort to runtime-deprecate this (if that's ever possible). If it's working fine, I don't see why it has to be deprecated?

@devsnek
Copy link
Member Author

devsnek commented Dec 10, 2019

@joyeecheung it's only docs deprecated. I don't think it would ever be feasible to runtime deprecate it. The idea of this PR is to provide a new API which doesn't act as confusingly (and somewhat dangerously) as the current API, and encourage people to use it.

@devsnek devsnek removed the wip Issues and PRs that are still a work in progress. label Dec 26, 2019
@devsnek
Copy link
Member Author

devsnek commented Dec 26, 2019

I'm still wondering about a globalProxy argument for new vm.Context. It could be useful, but due to V8's extremely odd implementation of the global proxy, it's quite awkward to work with.

@devsnek devsnek force-pushed the new-vm-context branch 2 times, most recently from 85acc04 to 0916064 Compare December 27, 2019 07:16
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

This needs a rebase.

doc/api/vm.md Show resolved Hide resolved
test/parallel/test-vm-context-behavior.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jan 11, 2020

@nodejs/collaborators This needs reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet