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: misleading language #10697

Closed
mscdex opened this issue Jan 8, 2017 · 6 comments
Closed

vm: misleading language #10697

mscdex opened this issue Jan 8, 2017 · 6 comments
Labels
good first issue Issues that are suitable for first-time contributors. vm Issues and PRs related to the vm subsystem.

Comments

@mscdex
Copy link
Contributor

mscdex commented Jan 8, 2017

  • Version: v4.x, v6.x, v7.x, master
  • Platform: n/a
  • Subsystem: doc

The note following the http.Server example in the vm documentation contains misleading language. Specifically, it mentions "the calling thread's context" when it has nothing to do with threading since code execution via vm happens on the same thread. The note makes it sound as if this is not the case.

I'm not sure what wording would describe this more accurately. "the parent context?" "the original context?"

@mscdex mscdex added good first issue Issues that are suitable for first-time contributors. vm Issues and PRs related to the vm subsystem. labels Jan 8, 2017
@aqrln
Copy link
Contributor

aqrln commented Jan 9, 2017

@mscdex what about just "the caller's context"?

@mscdex
Copy link
Contributor Author

mscdex commented Jan 9, 2017

I'd probably be okay with that too.

@aqrln
Copy link
Contributor

aqrln commented Jan 9, 2017

Or, even better, taking the previous sentence into account:

Note: The require() in the above case shares the state with context it is
passed from. This may introduce risks when untrusted code is executed, e.g.
altering objects from the this context in unwanted ways.

It is as simple as possible and it's not misleading like my previous suggestion actually is, too, after some thinking about it. (The word "caller" sounds like "the caller of require" which is certainly not the case since it may be called from another sandbox).

@mscdex
Copy link
Contributor Author

mscdex commented Jan 9, 2017

Yep :-) (although s/state with context/state with the context/ and s/the this context/the context/)

@aqrln
Copy link
Contributor

aqrln commented Jan 9, 2017

Yeah, that's even better. I'll open a PR in a moment.

@aqrln
Copy link
Contributor

aqrln commented Jan 9, 2017

@mscdex and "the this" is a typo, I meant just "this" :)

aqrln added a commit to aqrln/node that referenced this issue Jan 11, 2017
The note following the http.Server example in the vm documentation
contains misleading language. This commit removes the incorrect
reference to threads.

Fixes: nodejs#10697
@mscdex mscdex closed this as completed in 8781e61 Jan 11, 2017
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
The note following the http.Server example in the vm documentation
contains misleading language. This commit removes the incorrect
reference to threads.

Fixes: nodejs#10697
PR-URL: nodejs#10708
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 23, 2017
The note following the http.Server example in the vm documentation
contains misleading language. This commit removes the incorrect
reference to threads.

Fixes: nodejs#10697
PR-URL: nodejs#10708
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 24, 2017
The note following the http.Server example in the vm documentation
contains misleading language. This commit removes the incorrect
reference to threads.

Fixes: nodejs#10697
PR-URL: nodejs#10708
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 27, 2017
The note following the http.Server example in the vm documentation
contains misleading language. This commit removes the incorrect
reference to threads.

Fixes: nodejs#10697
PR-URL: nodejs#10708
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 7, 2017
The note following the http.Server example in the vm documentation
contains misleading language. This commit removes the incorrect
reference to threads.

Fixes: #10697
PR-URL: #10708
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
The note following the http.Server example in the vm documentation
contains misleading language. This commit removes the incorrect
reference to threads.

Fixes: #10697
PR-URL: #10708
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants