Skip to content

vm: introduce vm.Context#30709

Closed
devsnek wants to merge 2 commits into
nodejs:masterfrom
devsnek:new-vm-context
Closed

vm: introduce vm.Context#30709
devsnek wants to merge 2 commits into
nodejs:masterfrom
devsnek:new-vm-context

Conversation

@devsnek

@devsnek devsnek commented Nov 28, 2019

Copy link
Copy Markdown
Member

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

devsnek commented Nov 28, 2019

Copy link
Copy Markdown
Member Author

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

devsnek commented Dec 5, 2019

Copy link
Copy Markdown
Member Author

@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

joyeecheung commented Dec 10, 2019

Copy link
Copy Markdown
Member

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

devsnek commented Dec 10, 2019

Copy link
Copy Markdown
Member Author

@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

devsnek commented Dec 26, 2019

Copy link
Copy Markdown
Member Author

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

BridgeAR commented Jan 2, 2020

Copy link
Copy Markdown
Member

This needs a rebase.

Comment thread doc/api/vm.md Outdated
Comment thread test/parallel/test-vm-context-behavior.js Outdated
@fhinkel fhinkel removed their request for review January 9, 2020 16:28
@Trott

Trott commented Jan 11, 2020

Copy link
Copy Markdown
Member

@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