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.

Sign up for GitHub

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

Jump to bottom

doc: vm.runIn*Context can accept a string as options #19910

Closed
wants to merge 1 commit into from
Closed

doc: vm.runIn*Context can accept a string as options #19910

wants to merge 1 commit into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Apr 10, 2018

Checklist

see e.g.

node/lib/vm.js

Lines 267 to 273 in fa2d43b

function runInContext(code, contextifiedSandbox, options) {
validateContext(contextifiedSandbox);
if (typeof options === 'string') {
options = {
filename: options,
[kParsingContext]: contextifiedSandbox
};

@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 Apr 10, 2018
@bnoordhuis
Copy link
Member

For, hah, context: it's a backwards compatibility thing with the old vm module that was replaced in v0.11.x in 02fde58.

I'm ambivalent as to whether it should be documented. On the one hand, documentation should be complete. On the other hand, it shouldn't get bogged down in trivia or overload readers with options to choose from.

@Flarna
Copy link
Member Author

Flarna commented Apr 10, 2018

It is for sure used in the wild (see DefinitelyTyped/DefinitelyTyped#24798) simply because there was no need to adapt when coming from NodeJS 0.10.

I expect it's only possible to deprecate something which is documented - so maybe start with step one of deprecation 😁 here.

@vsemozhetbyt
Copy link
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/480/

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 10, 2018
@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2018
@trivikr
Copy link
Member

trivikr commented Apr 12, 2018

Landed in 7ead832

@trivikr trivikr closed this Apr 12, 2018
trivikr pushed a commit that referenced this pull request Apr 12, 2018
@Flarna @trivikr
doc: vm.runIn*Context can accept a string as options
7ead832 
PR-URL: #19910
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Apr 12, 2018
@Flarna @targos
doc: vm.runIn*Context can accept a string as options
5d9a7f8 
PR-URL: #19910
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Flarna Flarna deleted the vm_doc_stringopt branch April 12, 2018 17:43
jasnell pushed a commit that referenced this pull request Apr 16, 2018
@Flarna @jasnell
doc: vm.runIn*Context can accept a string as options
a60e498 
PR-URL: #19910
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Flarna @bnoordhuis @vsemozhetbyt @trivikr @jasnell @TimothyGu @cjihrig @devsnek @BridgeAR @nodejs-github-bot