Repository navigation
Add string option for vm.runIn*Context methods in Node.js typing - #24798
Conversation
| export function isContext(sandbox: Context): boolean; | ||
| export function runInContext(code: string, contextifiedSandbox: Context, options?: RunningScriptOptions): any; | ||
| export function runInContext(code: string, contextifiedSandbox: Context, options?: RunningScriptOptions | string): any; | ||
| /** @deprecated */ |
There was a problem hiding this comment.
typescript-bot
added
the
Popular package
|
@mohsen1 Thank you for submitting this PR! 🔔 @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @OliverJAsh @eps1lon @Hannes-Magnusson-CK @jkomyno @hoo29 @n-e @ajafff - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
|
@mohsen1 The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
Even this is in source code it's not in API documentation. I think this is NodeJS 0.10 API compatibility which had only string but no options object in API. On the other hand it is in code now and also on master so for NodeJs 8 and 9 it will be for sure not removed.... |
|
The type definition is marking existing and working core as incorrect. The definition should reflect the API even if not documented |
Flarna
left a comment
Flarna
left a comment
There was a problem hiding this comment.
Could you please add the test also for v9?
|
I still think that a PR towards NodeJS doc would be nice (like it was done for fs in nodejs/node#1806) but this is not part of this PR. |
|
🔔 @Flarna - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
|
A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
…initelyTyped#24798) * Add string option for vm.runIn*Context methods * Add test * lint
4 participants
This is part of my work at webpack/webpack#6862
Link to code
https://github.com/nodejs/node/blob/77b52fd58f7398a81999c81afd21fe2e156c0766/lib/vm.js#L284-L286
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.