FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

vm: introduce vm.Context by devsnek · Pull Request #30709 · nodejs/node · GitHub

/ node Public

vm: introduce vm.Context - #30709

Closed
devsnek wants to merge 2 commits into
nodejs:masterfrom
devsnek:new-vm-context
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

devsnek commented Nov 28, 2019 •
edited
Loading

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 added the vm Issues and PRs related to the vm subsystem. label Nov 28, 2019
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Nov 28, 2019
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 involving general changes in the lib/ or src/ directories. labels Nov 28, 2019

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?

This comment has been minimized.

Sign in to view
devsnek force-pushed the new-vm-context branch 3 times, most recently from f69a810 to 55e15ff Compare December 4, 2019 18:25

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 commented Dec 10, 2019 •
edited
Loading

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 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 removed the wip Issues and PRs that are still a work in progress. label Dec 26, 2019

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 force-pushed the new-vm-context branch 2 times, most recently from 85acc04 to 0916064 Compare December 27, 2019 07:16

BridgeAR commented Jan 2, 2020

Copy link
Copy Markdown
Member

This needs a rebase.

legendecas reviewed Jan 2, 2020
View reviewed changes
Comment thread doc/api/vm.md Outdated
Comment thread test/parallel/test-vm-context-behavior.js Outdated
fhinkel removed their request for review January 9, 2020 16:28
devsnek mentioned this pull request Jan 9, 2020
Merged
4 tasks

Trott commented Jan 11, 2020

Copy link
Copy Markdown
Member

@nodejs/collaborators This needs reviews.

This was referenced Sep 18, 2021
Open
Open
Dmarch28 mentioned this pull request May 13, 2022
Open
aliceUnhinged613 mentioned this pull request May 13, 2022
Open
137717unity mentioned this pull request May 13, 2022
Open
baby636 mentioned this pull request May 13, 2022
Open
kissofire mentioned this pull request May 13, 2022
Open
snyk-bot mentioned this pull request May 13, 2022
Open
ron-a20 mentioned this pull request May 13, 2022
Open
molllyn1 mentioned this pull request May 14, 2022
Open
snyk-bot mentioned this pull request Sep 25, 2022
Open
wedataintelligence mentioned this pull request Sep 26, 2022
Open
137717unity mentioned this pull request Sep 26, 2022
Open
snyk-bot mentioned this pull request Sep 27, 2022
Open
wedataintelligence mentioned this pull request Sep 29, 2022
Open
This was referenced Sep 29, 2022
Open
Open
This was referenced Oct 5, 2022
Open
Open
snyk-bot mentioned this pull request Oct 6, 2022
Open
137717unity mentioned this pull request Oct 7, 2022
Open
wedataintelligence mentioned this pull request Oct 7, 2022
Open
This was referenced Oct 8, 2022
Open
Open
137717unity mentioned this pull request Oct 9, 2022
Open
snyk-bot mentioned this pull request Oct 9, 2022
Open
a-parser mentioned this pull request Aug 22, 2023
Closed
aliceUnhinged613 mentioned this pull request Nov 25, 2023
Open
wedataintelligence mentioned this pull request Nov 28, 2023
Open
baby636 mentioned this pull request Nov 28, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis left review comments
Trott Trott left review comments
legendecas legendecas left review comments
+4 more reviewers
TimothyGu TimothyGu left review comments
ExE-Boss ExE-Boss left review comments
lundibundi lundibundi left review comments
vdeturckheim vdeturckheim approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone
Back | FazBrowse Home | New Git URL