Match ts-node REPL behavior of object literals with Node REPL (#1699)

* Implement unparenthesized REPL object literals * Fix property access error inconsistency * Run prettier on src/repl.ts * Add cross-env as dev dependency * Fix issue preventing tests from running on envs with NODE_PATH set * Silence deprecation warning spam on tests * Single quotes caused some cli bugs * Add REPL object literal tests * remove cross-env * Add NODE_PATH check and warning to tests runner See: #1697 (comment) * Run prettier on repl.spec.ts * node nightly tests fail because of unexpected custom ESM loaders warnings so fix that i guess? * fix tests on TS 2.7 * node nightly tests still failed, fix attempt 2 * append instead of overriding NODE_OPTIONS * accept warning-only stderr on nightly test * if check didnt fire * maybe the regex is broken * am i even editing the right lines * make test-cov match test-spec * try checking for nightly again... * tests work! clean them up now, please don't break * Remove node nightly tests warning checks (superseded by #1701) * simplify NODE_PATH check * attempt at running new repl tests in-process for speed * switch tests to run in-process using existing macros * finish changes to run tests in-process Co-authored-by: Andrew Bradley <cspotcode@gmail.com>
TypeStrong · Apr 20, 2022 · 9e25557 · 9e25557
1 parent d7670e9
commit 9e25557
Show file tree

Hide file tree

Showing 5 changed files with 182 additions and 13 deletions.
diff --git a/ava.config.cjs b/ava.config.cjs
@@ -11,6 +11,7 @@ module.exports = {
     // This avoids passing it to spawned processes under test, which would negatively affect
     // their behavior.
     FORCE_COLOR: '3',
+    NODE_PATH: ''
   },
   require: ['./src/test/remove-env-var-force-color.js'],
   timeout: '300s',
@@ -21,7 +22,12 @@ module.exports = {
   /*
    * Tests *must* install and use our most recent ts-node tarball.
    * We must prevent them from accidentally require-ing a different version of
-   * ts-node, from either node_modules or tests/node_modules
+   * ts-node, from either node_modules or tests/node_modules.
+   *
+   * Another possibility of interference is NODE_PATH environment variable being set,
+   * and ts-node being installed in any of the paths listed on NODE_PATH, to fix this,
+   * the NODE_PATH variable must be removed from the environment *BEFORE* running ava.
+   * An error will be thrown when trying to run tests with NODE_PATH set to paths with ts-node installed.
    */
 
   const { existsSync, rmSync } = require('fs');
@@ -32,7 +38,20 @@ module.exports = {
   remove(resolve(__dirname, 'tests/node_modules/ts-node'));
 
   // Prove that we did it correctly
-  expect(() => {createRequire(resolve(__dirname, 'tests/foo.js')).resolve('ts-node')}).toThrow();
+  (() => {
+    let resolved;
+    try {
+      resolved = createRequire(resolve(__dirname, 'tests/foo.js')).resolve('ts-node');
+    } catch(err) {return}
+
+    // require.resolve() found ts-node; this should not happen.
+    let errorMessage = `require.resolve('ts-node') unexpectedly resolved to ${resolved}\n`;
+    // Check for NODE_PATH interference. See comments above.
+    if(process.env.NODE_PATH) {
+      errorMessage += `NODE_PATH environment variable is set.  This test suite does not support running with NODE_PATH.  Unset it before running the tests.`;
+    }
+    throw new Error(errorMessage);
+  })();
 
   function remove(p) {
     // Avoid node deprecation warning triggered by rimraf

diff --git a/package.json b/package.json
@@ -171,6 +171,9 @@
     "v8-compile-cache-lib": "^3.0.1",
     "yn": "3.1.1"
   },
+  "prettier": {
+    "singleQuote": true
+  },
   "volta": {
     "node": "17.5.0",
     "npm": "6.14.15"

diff --git a/src/repl.ts b/src/repl.ts
@@ -509,17 +509,30 @@ function appendCompileAndEvalInput(options: {
   service: Service;
   state: EvalState;
   input: string;
+  wrappedErr?: unknown;
   /** Enable top-level await but only if the TSNode service allows it. */
   enableTopLevelAwait?: boolean;
   context: Context | undefined;
 }): AppendCompileAndEvalInputResult {
   const {
     service,
     state,
-    input,
+    wrappedErr,
     enableTopLevelAwait = false,
     context,
   } = options;
+  let { input } = options;
+
+  // It's confusing for `{ a: 1 }` to be interpreted as a block statement
+  // rather than an object literal. So, we first try to wrap it in
+  // parentheses, so that it will be interpreted as an expression.
+  // Based on https://github.com/nodejs/node/blob/c2e6822153bad023ab7ebd30a6117dcc049e475c/lib/repl.js#L413-L422
+  let wrappedCmd = false;
+  if (!wrappedErr && /^\s*{/.test(input) && !/;\s*$/.test(input)) {
+    input = `(${input.trim()})\n`;
+    wrappedCmd = true;
+  }
+
   const lines = state.lines;
   const isCompletion = !/\n$/.test(input);
   const undo = appendToEvalState(state, input);
@@ -536,6 +549,20 @@ function appendCompileAndEvalInput(options: {
     output = service.compile(state.input, state.path, -lines);
   } catch (err) {
     undo();
+
+    if (wrappedCmd) {
+      if (err instanceof TSError && err.diagnosticCodes[0] === 2339) {
+        // Ensure consistent and more sane behavior between { a: 1 }['b'] and ({ a: 1 }['b'])
+        throw err;
+      }
+      // Unwrap and try again
+      return appendCompileAndEvalInput({
+        ...options,
+        wrappedErr: err,
+      });
+    }
+
+    if (wrappedErr) throw wrappedErr;
     throw err;
   }
 
@@ -689,6 +716,10 @@ const RECOVERY_CODES: Map<number, Set<number> | null> = new Map([
   [1005, null], // "')' expected.", "'}' expected."
   [1109, null], // "Expression expected."
   [1126, null], // "Unexpected end of text."
+  [
+    1136, // "Property assignment expected."
+    new Set([1005]), // happens when typing out an object literal or block scope across multiple lines: '{ foo: 123,'
+  ],
   [1160, null], // "Unterminated template literal."
   [1161, null], // "Unterminated regular expression literal."
   [2355, null], // "A function whose declared type is neither 'void' nor 'any' must return a value."

diff --git a/src/test/helpers.ts b/src/test/helpers.ts
@@ -202,7 +202,8 @@ export function getStream(stream: Readable, waitForPattern?: string | RegExp) {
 //#region Reset node environment
 
 const defaultRequireExtensions = captureObjectState(require.extensions);
-const defaultProcess = captureObjectState(process);
+// Avoid node deprecation warning for accessing _channel
+const defaultProcess = captureObjectState(process, ['_channel']);
 const defaultModule = captureObjectState(require('module'));
 const defaultError = captureObjectState(Error);
 const defaultGlobal = captureObjectState(global);
@@ -214,15 +215,20 @@ const defaultGlobal = captureObjectState(global);
  * Must also play nice with `nyc`'s environmental mutations.
  */
 export function resetNodeEnvironment() {
+  const sms =
+    require('@cspotcode/source-map-support') as typeof import('@cspotcode/source-map-support');
   // We must uninstall so that it resets its internal state; otherwise it won't know it needs to reinstall in the next test.
-  require('@cspotcode/source-map-support').uninstall();
+  sms.uninstall();
+  // Must remove handlers to avoid a memory leak
+  sms.resetRetrieveHandlers();
 
   // Modified by ts-node hooks
   resetObject(require.extensions, defaultRequireExtensions);
 
   // ts-node attaches a property when it registers an instance
   // source-map-support monkey-patches the emit function
-  resetObject(process, defaultProcess);
+  // Avoid node deprecation warnings for setting process.config or accessing _channel
+  resetObject(process, defaultProcess, undefined, ['_channel'], ['config']);
 
   // source-map-support swaps out the prepareStackTrace function
   resetObject(Error, defaultError);
@@ -235,11 +241,10 @@ export function resetNodeEnvironment() {
   resetObject(global, defaultGlobal, ['__coverage__']);
 }
 
-function captureObjectState(object: any) {
+function captureObjectState(object: any, avoidGetters: string[] = []) {
   const descriptors = Object.getOwnPropertyDescriptors(object);
   const values = mapValues(descriptors, (_d, key) => {
-    // Avoid node deprecation warning for accessing _channel
-    if (object === process && key === '_channel') return descriptors[key].value;
+    if (avoidGetters.includes(key)) return descriptors[key].value;
     return object[key];
   });
   return {
@@ -251,7 +256,9 @@ function captureObjectState(object: any) {
 function resetObject(
   object: any,
   state: ReturnType<typeof captureObjectState>,
-  doNotDeleteTheseKeys: string[] = []
+  doNotDeleteTheseKeys: string[] = [],
+  doNotSetTheseKeys: string[] = [],
+  avoidSetterIfUnchanged: string[] = []
 ) {
   const currentDescriptors = Object.getOwnPropertyDescriptors(object);
   for (const key of Object.keys(currentDescriptors)) {
@@ -262,9 +269,8 @@ function resetObject(
   // Trigger nyc's setter functions
   for (const [key, value] of Object.entries(state.values)) {
     try {
-      // Avoid node deprecation warnings for setting process.config or accessing _channel
-      if (object === process && key === '_channel') continue;
-      if (object === process && key === 'config' && object[key] === value)
+      if (doNotSetTheseKeys.includes(key)) continue;
+      if (avoidSetterIfUnchanged.includes(key) && object[key] === value)
         continue;
       object[key] = value;
     } catch {}

diff --git a/src/test/repl/repl.spec.ts b/src/test/repl/repl.spec.ts
@@ -16,9 +16,13 @@ import {
 } from './helpers';
 
 const test = context(ctxTsNode).context(ctxRepl);
+test.runSerially();
 test.beforeEach(async (t) => {
   t.teardown(() => {
     resetNodeEnvironment();
+    // Useful for debugging memory leaks.  Leaving in case I need it again.
+    // global.gc(); // Requires adding nodeArguments: ['--expose-gc'] to ava config
+    // console.dir(process.memoryUsage().heapUsed / 1000 / 1000);
   });
 });
 
@@ -540,3 +544,109 @@ test.suite('REPL declares types for node built-ins within REPL', (test) => {
     expect(stderr).toBe('');
   });
 });
+
+test.suite('REPL treats object literals and block scopes correctly', (test) => {
+  test(
+    'repl should treat { key: 123 } as object literal',
+    macroReplNoErrorsAndStdoutContains,
+    '{ key: 123 }',
+    '{ key: 123 }'
+  );
+  test(
+    'repl should treat ({ key: 123 }) as object literal',
+    macroReplNoErrorsAndStdoutContains,
+    '({ key: 123 })',
+    '{ key: 123 }'
+  );
+  test(
+    'repl should treat ({ let v = 0; v; }) as object literal and error',
+    macroReplStderrContains,
+    '({ let v = 0; v; })',
+    semver.satisfies(ts.version, '2.7')
+      ? 'error TS2304'
+      : 'No value exists in scope for the shorthand property'
+  );
+  test(
+    'repl should treat { let v = 0; v; } as block scope',
+    macroReplNoErrorsAndStdoutContains,
+    '{ let v = 0; v; }',
+    '0'
+  );
+  test.suite('extra', (test) => {
+    test.skipIf(semver.satisfies(ts.version, '2.7'));
+    test(
+      'repl should treat { key: 123 }; as block scope',
+      macroReplNoErrorsAndStdoutContains,
+      '{ key: 123 };',
+      '123'
+    );
+    test(
+      'repl should treat {\\nkey: 123\\n}; as block scope',
+      macroReplNoErrorsAndStdoutContains,
+      '{\nkey: 123\n};',
+      '123'
+    );
+    test(
+      'repl should treat { key: 123 }[] as block scope (edge case)',
+      macroReplNoErrorsAndStdoutContains,
+      '{ key: 123 }[]',
+      '[]'
+    );
+  });
+  test.suite('multiline', (test) => {
+    test(
+      'repl should treat {\\nkey: 123\\n} as object literal',
+      macroReplNoErrorsAndStdoutContains,
+      '{\nkey: 123\n}',
+      '{ key: 123 }'
+    );
+    test(
+      'repl should treat ({\\nkey: 123\\n}) as object literal',
+      macroReplNoErrorsAndStdoutContains,
+      '({\nkey: 123\n})',
+      '{ key: 123 }'
+    );
+    test(
+      'repl should treat ({\\nlet v = 0;\\nv;\\n}) as object literal and error',
+      macroReplStderrContains,
+      '({\nlet v = 0;\nv;\n})',
+      semver.satisfies(ts.version, '2.7')
+        ? 'error TS2304'
+        : 'No value exists in scope for the shorthand property'
+    );
+    test(
+      'repl should treat {\\nlet v = 0;\\nv;\\n} as block scope',
+      macroReplNoErrorsAndStdoutContains,
+      '{\nlet v = 0;\nv;\n}',
+      '0'
+    );
+  });
+  test.suite('property access', (test) => {
+    test(
+      'repl should treat { key: 123 }.key as object literal property access',
+      macroReplNoErrorsAndStdoutContains,
+      '{ key: 123 }.key',
+      '123'
+    );
+    test(
+      'repl should treat { key: 123 }["key"] as object literal indexed access',
+      macroReplNoErrorsAndStdoutContains,
+      '{ key: 123 }["key"]',
+      '123'
+    );
+    test(
+      'repl should treat { key: 123 }.foo as object literal non-existent property access',
+      macroReplStderrContains,
+      '{ key: 123 }.foo',
+      "Property 'foo' does not exist on type"
+    );
+    test(
+      'repl should treat { key: 123 }["foo"] as object literal non-existent indexed access',
+      macroReplStderrContains,
+      '{ key: 123 }["foo"]',
+      semver.satisfies(ts.version, '2.7')
+        ? 'error TS7017'
+        : "Property 'foo' does not exist on type"
+    );
+  });
+});