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

doc: improve documentation for the vm module #16867

Closed
wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Nov 7, 2017

I think an intro section would help the vm documentation.

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

doc

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem. labels Nov 7, 2017
@fhinkel fhinkel force-pushed the nov/explain-contextify branch from 33b1644 to 73470c9 Compare November 7, 2017 18:13
doc/api/vm.md Outdated
compiled, saved, and run later.

A common use case is to run the code in a sandboxed environment.
Then the sandboxed code uses a different
Copy link
Member

@bengl bengl Nov 7, 2017

Choose a reason for hiding this comment

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

Maybe:

The sandboxed code uses a different V8 Context, meaning...

doc/api/vm.md Outdated
sandbox object.
The sandboxed code treats any property on the sandbox like a
global variable. Any changes on global variables caused by the sandboxed
code are recorded on the sandbox object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "recorded on" -> "reflected in"

@fhinkel fhinkel force-pushed the nov/explain-contextify branch from 73470c9 to 3313580 Compare November 7, 2017 20:48
doc/api/vm.md Outdated

A common use case is to run the code in a sandboxed environment.
Then the sandboxed code uses a different V8 Context, meaning that
the it has a different global object than the rest of the code.
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: the it -> it

@fhinkel fhinkel force-pushed the nov/explain-contextify branch from 3313580 to ed0f7c8 Compare November 8, 2017 06:14
doc/api/vm.md Outdated
The sandboxed code uses a different V8 Context, meaning that
it has a different global object than the rest of the code.

You can provide the context by ["contextifying"][contextified] a sandbox
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please avoid the use of informal pronouns like you in the docs :-)

Add an intro section and example for the vm module.
@fhinkel fhinkel force-pushed the nov/explain-contextify branch from ed0f7c8 to 76276f0 Compare November 8, 2017 18:44
@fhinkel
Copy link
Member Author

fhinkel commented Nov 9, 2017

Thanks for the reviews. Landed in 5e1e460

@fhinkel fhinkel closed this Nov 9, 2017
fhinkel added a commit that referenced this pull request Nov 9, 2017
Add an intro section and example for the vm module.

PR-URL: #16867
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
Add an intro section and example for the vm module.

PR-URL: #16867
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Add an intro section and example for the vm module.

PR-URL: #16867
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Add an intro section and example for the vm module.

PR-URL: #16867
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins
Copy link
Contributor

@fhinkel I've landed this on both v6.x and v8.x

please lmk if this isn't appropriate for those release lines

@gibfahn gibfahn mentioned this pull request Nov 21, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Add an intro section and example for the vm module.

PR-URL: #16867
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Add an intro section and example for the vm module.

PR-URL: #16867
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants