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

src: explain implementation of vm module #16962

Closed
wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Nov 12, 2017

The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person (and future me) working on this.

Checklist
  • make lint passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

src - but only adding a comment

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Nov 12, 2017
@@ -64,8 +64,29 @@ using v8::UnboundScript;
using v8::Value;
using v8::WeakCallbackInfo;

// The vm module exectues code in a sandboxed environment with a different
Copy link
Contributor

Choose a reason for hiding this comment

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

exectues -> executes

@@ -64,8 +64,29 @@ using v8::UnboundScript;
using v8::Value;
using v8::WeakCallbackInfo;

// The vm module exectues code in a sandboxed environment with a different
// global object than the rest of the code. This is archieved by applying
Copy link
Member

Choose a reason for hiding this comment

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

archieved -> achieved

The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person working on this.
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM, this shouldn't need to wait 48 hours to land.

@jasnell
Copy link
Member

jasnell commented Nov 12, 2017

Since this is only adding a comment, just going to lint: https://ci.nodejs.org/job/node-test-linter/13499/

jasnell pushed a commit that referenced this pull request Nov 12, 2017
The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person working on this.

PR-URL: #16962
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member

jasnell commented Nov 12, 2017

Landed in f0c674d

@jasnell jasnell closed this Nov 12, 2017
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person working on this.

PR-URL: #16962
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@MylesBorins
Copy link
Member

Should this be backported to v6.x-staging or v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants