From 9e255575aa44a8af3cb01f4d810f8480541fe943 Mon Sep 17 00:00:00 2001 From: jhmaster2000 <32803471+jhmaster2000@users.noreply.github.com> Date: Wed, 20 Apr 2022 00:18:10 -0300 Subject: [PATCH] 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: https://github.com/TypeStrong/ts-node/issues/1697#issuecomment-1080258594 * 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 --- ava.config.cjs | 23 +++++++- package.json | 3 + src/repl.ts | 33 ++++++++++- src/test/helpers.ts | 26 +++++---- src/test/repl/repl.spec.ts | 110 +++++++++++++++++++++++++++++++++++++ 5 files changed, 182 insertions(+), 13 deletions(-) diff --git a/ava.config.cjs b/ava.config.cjs index 4286a1474..b3a5904b7 100644 --- 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 index 35887a910..f48dda011 100644 --- 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 index ac3579817..69d02425c 100644 --- a/src/repl.ts +++ b/src/repl.ts @@ -509,6 +509,7 @@ 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; @@ -516,10 +517,22 @@ function appendCompileAndEvalInput(options: { 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 | 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 index 5d47d504b..5856959f4 100644 --- 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, - 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 index 7fa263148..55f49cb68 100644 --- 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" + ); + }); +});