From 7d3b123afc57d8461c2b7b0d20045da7473654bc Mon Sep 17 00:00:00 2001
From: Domenic Denicola <d@domenic.me>
Date: Wed, 1 Jan 2020 17:41:07 -0500
Subject: [PATCH] Replace runVMScript() with getInternalVMContext()

This allows more flexibility for vm-using consumers. Closes #2731.
---
 README.md                                     | 20 +++++++++-------
 lib/api.js                                    |  4 ++--
 .../living/helpers/create-event-accessor.js   |  3 +--
 test/api/methods.js                           | 24 ++++++++++---------
 4 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/README.md b/README.md
index 69fa1d0d93..2d09dff0db 100644
--- a/README.md
+++ b/README.md
@@ -104,7 +104,7 @@ This is turned off by default for performance reasons, but is safe to enable.
 
 Note that we strongly advise against trying to "execute scripts" by mashing together the jsdom and Node global environments (e.g. by doing `global.window = dom.window`), and then executing scripts or test code inside the Node global environment. Instead, you should treat jsdom like you would a browser, and run all scripts and tests that need access to a DOM inside the jsdom environment, using `window.eval` or `runScripts: "dangerously"`. This might require, for example, creating a browserify bundle to execute as a `<script>` element—just like you would in a browser.
 
-Finally, for advanced use cases you can use the `dom.runVMScript(script)` method, documented below.
+Finally, for advanced use cases you can use the `dom.getInternalVMContext()` method, documented below.
 
 ### Pretending to be a visual browser
 
@@ -309,9 +309,11 @@ console.log(dom.nodeLocation(imgEl));    // { startOffset: 13, endOffset: 32 }
 
 Note that this feature only works if you have set the `includeNodeLocations` option; node locations are off by default for performance reasons.
 
-### Running vm-created scripts with `runVMScript(script[, options])`
+### Interfacing with the Node.js `vm` module using `getInternalVMContext()`
 
-The built-in `vm` module of Node.js allows you to create `Script` instances, which can be compiled ahead of time and then run multiple times on a given "VM context". Behind the scenes, a jsdom `Window` is indeed a VM context. To get access to this ability, use the `runVMScript()` method:
+The built-in [`vm`](https://nodejs.org/api/vm.html) module of Node.js is what underpins jsdom's script-running magic. Some advanced use cases, like pre-compiling a script and then running it multiple times, benefit from using the `vm` module directly with a jsdom-created `Window`.
+
+To get access to the [contextified global object](https://nodejs.org/api/vm.html#vm_what_does_it_mean_to_contextify_an_object), suitable for use with the `vm` APIs, you can use the `getInternalVMContext()` method:
 
 ```js
 const { Script } = require("vm");
@@ -325,16 +327,18 @@ const s = new Script(`
   ++this.ran;
 `);
 
-dom.runVMScript(s);
-dom.runVMScript(s);
-dom.runVMScript(s);
+const vmContext = dom.getInternalVMContext();
+
+script.runInContext(vmContext);
+script.runInContext(vmContext);
+script.runInContext(vmContext);
 
-dom.window.ran === 3;
+console.assert(dom.window.ran === 3);
 ```
 
 This is somewhat-advanced functionality, and we advise sticking to normal DOM APIs (such as `window.eval()` or `document.createElement("script")`) unless you have very specific needs.
 
-`runVMScript()` also takes an `options` object as its second argument. See the [Node.js docs](https://nodejs.org/api/vm.html#vm_script_runincontext_contextifiedsandbox_options) for details. (This functionality does not work when [using jsdom in a web browser](#running-jsdom-inside-a-web-browser).)
+Note that this method will throw an exception if the `JSDOM` instance was created without `runScripts` set, or if you are [using jsdom in a web browser](#running-jsdom-inside-a-web-browser).
 
 ### Reconfiguring the jsdom with `reconfigure(settings)`
 
diff --git a/lib/api.js b/lib/api.js
index 3194e9cdc7..440a640482 100644
--- a/lib/api.js
+++ b/lib/api.js
@@ -71,13 +71,13 @@ class JSDOM {
     return idlUtils.implForWrapper(node).sourceCodeLocation;
   }
 
-  runVMScript(script, options) {
+  getInternalVMContext() {
     if (!vm.isContext(this[window])) {
       throw new TypeError("This jsdom was not configured to allow script running. " +
         "Use the runScripts option during creation.");
     }
 
-    return script.runInContext(this[window], options);
+    return this[window];
   }
 
   reconfigure(settings) {
diff --git a/lib/jsdom/living/helpers/create-event-accessor.js b/lib/jsdom/living/helpers/create-event-accessor.js
index ee6b82021c..4890b807ab 100644
--- a/lib/jsdom/living/helpers/create-event-accessor.js
+++ b/lib/jsdom/living/helpers/create-event-accessor.js
@@ -1,6 +1,5 @@
 "use strict";
 
-const vm = require("vm");
 const conversions = require("webidl-conversions");
 const idlUtils = require("../generated/utils");
 const ErrorEvent = require("../generated/ErrorEvent");
@@ -119,7 +118,7 @@ exports.createEventAccessor = function createEventAccessor(obj, event) {
         // Note: the with (window) { } is not necessary in Node, but is necessary in a browserified environment.
 
         let fn;
-        const createFunction = vm.isContext(document._global) ? document.defaultView._globalProxy.Function : Function;
+        const createFunction = document.defaultView.Function;
         if (event === "error" && element === null) {
           const wrapperBody = document ? body + `\n//# sourceURL=${document.URL}` : body;
 
diff --git a/test/api/methods.js b/test/api/methods.js
index 0b6857aa22..739c8f0dba 100644
--- a/test/api/methods.js
+++ b/test/api/methods.js
@@ -135,12 +135,11 @@ describe("API: JSDOM class's methods", () => {
     });
   });
 
-  describe("runVMScript", () => {
+  describe("getInternalVMContext", { skipIfBrowser: true }, () => {
     it("should throw when runScripts is left as the default", () => {
       const dom = new JSDOM();
-      const script = new vm.Script("this.ran = true;");
 
-      assert.throws(() => dom.runVMScript(script), TypeError);
+      assert.throws(() => dom.getInternalVMContext(), TypeError);
 
       assert.strictEqual(dom.window.ran, undefined);
     });
@@ -149,7 +148,7 @@ describe("API: JSDOM class's methods", () => {
       const dom = new JSDOM(``, { runScripts: "outside-only" });
       const script = new vm.Script("this.ran = true;");
 
-      dom.runVMScript(script);
+      script.runInContext(dom.getInternalVMContext());
 
       assert.strictEqual(dom.window.ran, true);
     });
@@ -158,7 +157,7 @@ describe("API: JSDOM class's methods", () => {
       const dom = new JSDOM(``, { runScripts: "dangerously" });
       const script = new vm.Script("this.ran = true;");
 
-      dom.runVMScript(script);
+      script.runInContext(dom.getInternalVMContext());
 
       assert.strictEqual(dom.window.ran, true);
     });
@@ -167,7 +166,7 @@ describe("API: JSDOM class's methods", () => {
       const dom = new JSDOM(``, { runScripts: "outside-only" });
       const script = new vm.Script("5;");
 
-      const result = dom.runVMScript(script);
+      const result = script.runInContext(dom.getInternalVMContext());
 
       assert.strictEqual(result, 5);
     });
@@ -176,18 +175,21 @@ describe("API: JSDOM class's methods", () => {
       const dom = new JSDOM(``, { runScripts: "outside-only" });
       const script = new vm.Script("if (!this.ran) { this.ran = 0; } ++this.ran;");
 
-      dom.runVMScript(script);
-      dom.runVMScript(script);
-      dom.runVMScript(script);
+      script.runInContext(dom.getInternalVMContext());
+      script.runInContext(dom.getInternalVMContext());
+      script.runInContext(dom.getInternalVMContext());
 
       assert.strictEqual(dom.window.ran, 3);
     });
 
-    it("should allow passing through options", { skipIfBrowser: true }, () => {
+    it("should allow passing through options", () => {
       const dom = new JSDOM(``, { runScripts: "outside-only" });
       const script = new vm.Script("while(true) {}");
 
-      assert.throws(() => dom.runVMScript(script, { timeout: 50 }), /Script execution timed out(?: after 50ms|\.)/);
+      assert.throws(
+        () => script.runInContext(dom.getInternalVMContext(), { timeout: 50 }),
+        /Script execution timed out(?: after 50ms|\.)/
+      );
     });
   });