From 210aee28502bbd8068d9f7dc77af0379ec7a0495 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 4 May 2020 03:46:23 +0200 Subject: [PATCH 1/8] repl: improve repl preview This aligns the REPL preview with the one used in the Chrome DevTools console. It will now preview the output for the input including the completion suffix as input. When pressing enter while previewing such data, it will automatically insert the suffix before evaluating the input. When pressing escape, that behavior is deactivated until the input is changed. Signed-off-by: Ruben Bridgewater --- lib/internal/repl/utils.js | 44 ++++++++++++++----- lib/repl.js | 2 +- test/parallel/test-repl-history-navigation.js | 40 +++++++++++++++-- test/parallel/test-repl-preview.js | 6 ++- 4 files changed, 77 insertions(+), 15 deletions(-) diff --git a/lib/internal/repl/utils.js b/lib/internal/repl/utils.js index 476760a08d5934..a8b1e336d9aee8 100644 --- a/lib/internal/repl/utils.js +++ b/lib/internal/repl/utils.js @@ -138,6 +138,8 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { let wrapped = false; + let escaped = null; + function getPreviewPos() { const displayPos = repl._getDisplayPos(`${repl._prompt}${repl.line}`); const cursorPos = repl.line.length !== repl.cursor ? @@ -146,7 +148,13 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { return { displayPos, cursorPos }; } - const clearPreview = () => { + function isCursorAtInputEnd() { + const { cursorPos, displayPos } = getPreviewPos(); + return cursorPos.rows === displayPos.rows && + cursorPos.cols === displayPos.cols; + } + + const clearPreview = (key) => { if (inputPreview !== null) { const { displayPos, cursorPos } = getPreviewPos(); const rows = displayPos.rows - cursorPos.rows + 1; @@ -179,8 +187,23 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { cursorTo(repl.output, pos.cursorPos.cols); moveCursor(repl.output, 0, -rows); } + if (!key.ctrl && !key.shift) { + if (key.name === 'escape') { + if (escaped === null && key.meta) { + escaped = repl.line; + } + } else if ((key.name === 'return' || key.name === 'enter') && + !key.meta && + escaped !== repl.line && + isCursorAtInputEnd()) { + repl._insertString(completionPreview); + } + } completionPreview = null; } + if (escaped !== repl.line) { + escaped = null; + } }; function showCompletionPreview(line, insertPreview) { @@ -317,13 +340,6 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { } // Add the autocompletion preview. - // TODO(BridgeAR): Trigger the input preview after the completion preview. - // That way it's possible to trigger the input prefix including the - // potential completion suffix. To do so, we also have to change the - // behavior of `enter` and `escape`: - // Enter should automatically add the suffix to the current line as long as - // escape was not pressed. We might even remove the preview in case any - // cursor movement is triggered. const insertPreview = false; showCompletionPreview(repl.line, insertPreview); @@ -397,9 +413,17 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { moveCursor(repl.output, 0, -rows - 1); }; - getInputPreview(line, inputPreviewCallback); + let previewLine = line; + + if (completionPreview !== null && + isCursorAtInputEnd() && + escaped !== repl.line) { + previewLine += completionPreview; + } + + getInputPreview(previewLine, inputPreviewCallback); if (wrapped) { - getInputPreview(line, inputPreviewCallback); + getInputPreview(previewLine, inputPreviewCallback); } wrapped = false; }; diff --git a/lib/repl.js b/lib/repl.js index 3186e0a31b8c86..0f00632d35d515 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -849,7 +849,7 @@ function REPLServer(prompt, self.cursor === 0 && self.line.length === 0) { self.clearLine(); } - clearPreview(); + clearPreview(key); if (!reverseSearch(d, key)) { ttyWrite(d, key); showPreview(); diff --git a/test/parallel/test-repl-history-navigation.js b/test/parallel/test-repl-history-navigation.js index 8eda8e3ceceeef..f34de6a178fe68 100644 --- a/test/parallel/test-repl-history-navigation.js +++ b/test/parallel/test-repl-history-navigation.js @@ -55,6 +55,7 @@ const WORD_RIGHT = { name: 'right', ctrl: true }; const GO_TO_END = { name: 'end' }; const DELETE_WORD_LEFT = { name: 'backspace', ctrl: true }; const SIGINT = { name: 'c', ctrl: true }; +const ESCAPE = { name: 'escape', meta: true }; const prompt = '> '; const WAIT = '€'; @@ -180,8 +181,10 @@ const tests = [ 'veryLongName'.repeat(30), ENTER, `${'\x1B[90m \x1B[39m'.repeat(235)} fun`, + ESCAPE, ENTER, `${' '.repeat(236)} fun`, + ESCAPE, ENTER ], expected: [], @@ -316,6 +319,7 @@ const tests = [ env: { NODE_REPL_HISTORY: defaultHistoryPath }, showEscapeCodes: true, skip: !process.features.inspector, + checkTotal: true, test: [ 'fu', 'n', @@ -329,6 +333,12 @@ const tests = [ BACKSPACE, WORD_LEFT, WORD_RIGHT, + ESCAPE, + ENTER, + UP, + LEFT, + ENTER, + UP, ENTER ], // C = Cursor n forward @@ -377,12 +387,36 @@ const tests = [ '\x1B[0K', '\x1B[7D', '\x1B[10G', ' // n', '\x1B[3G', '\x1B[10G', // 10. Word right. Cleanup '\x1B[0K', '\x1B[3G', '\x1B[7C', ' // n', '\x1B[10G', - '\x1B[0K', - // 11. ENTER + // 11. ESCAPE + '\x1B[0K', ' // n', '\x1B[10G', '\x1B[0K', + // 12. ENTER '\r\n', 'Uncaught ReferenceError: functio is not defined\n', '\x1B[1G', '\x1B[0J', - prompt, '\x1B[3G', '\r\n' + // 13. UP + prompt, '\x1B[3G', '\x1B[1G', '\x1B[0J', + `${prompt}functio`, '\x1B[10G', + ' // n', '\x1B[10G', + ' // n', '\x1B[10G', + // 14. LEFT + '\x1B[0K', '\x1B[1D', + '\x1B[10G', ' // n', '\x1B[9G', '\x1B[10G', + // 15. ENTER + '\x1B[0K', '\x1B[9G', '\x1B[1C', + '\r\n', + 'Uncaught ReferenceError: functio is not defined\n', + '\x1B[1G', '\x1B[0J', + '> ', '\x1B[3G', + // 16. UP + '\x1B[1G', '\x1B[0J', + '> functio', '\x1B[10G', + ' // n', '\x1B[10G', + ' // n', '\x1B[10G', '\x1B[0K', + // 17. ENTER + 'n', '\r\n', + '\x1B[1G', '\x1B[0J', + '... ', '\x1B[5G', + '\r\n' ], clean: true }, diff --git a/test/parallel/test-repl-preview.js b/test/parallel/test-repl-preview.js index b8dbbf3f9a993d..3ec873b4c46188 100644 --- a/test/parallel/test-repl-preview.js +++ b/test/parallel/test-repl-preview.js @@ -90,7 +90,11 @@ async function tests(options) { input: 'koo', noPreview: '[Function: koo]', preview: [ - 'k\x1B[90moo\x1B[39m\x1B[9G\x1B[0Ko\x1B[90mo\x1B[39m\x1B[10G\x1B[0Ko', + 'k\x1B[90moo\x1B[39m\x1B[9G', + '\x1B[90m[Function: koo]\x1B[39m\x1B[9G\x1B[1A\x1B[1B\x1B[2K\x1B[1A' + + '\x1B[0Ko\x1B[90mo\x1B[39m\x1B[10G', + '\x1B[90m[Function: koo]\x1B[39m\x1B[10G\x1B[1A\x1B[1B\x1B[2K\x1B[1A' + + '\x1B[0Ko', '\x1B[90m[Function: koo]\x1B[39m\x1B[11G\x1B[1A\x1B[1B\x1B[2K\x1B[1A\r', '\x1B[36m[Function: koo]\x1B[39m' ] From 733baf1b2cb1d5152889f3ebeafe0c48e642c09c Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 4 May 2020 03:49:04 +0200 Subject: [PATCH 2/8] repl: show reference errors during preview This aligns the behavior with the one in the Firefox console. It will visualize ReferenceErrors in case the input has no possible completion and no buffered input. That way typos can already be highlighted before being evaluated. Signed-off-by: Ruben Bridgewater --- lib/internal/repl/utils.js | 17 +++++++++++++++-- lib/repl.js | 6 ++++-- test/parallel/test-repl-mode.js | 20 +++++++++++++++++--- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/lib/internal/repl/utils.js b/lib/internal/repl/utils.js index a8b1e336d9aee8..0a79fb44babf92 100644 --- a/lib/internal/repl/utils.js +++ b/lib/internal/repl/utils.js @@ -48,6 +48,8 @@ const previewOptions = { showHidden: false }; +const REPL_MODE_STRICT = Symbol('repl-strict'); + // If the error is that we've unexpectedly ended the input, // then let the user try to recover by adding more input. // Note: `e` (the original exception) is not used by the current implementation, @@ -136,6 +138,8 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { let previewCompletionCounter = 0; let completionPreview = null; + let hasCompletions = false; + let wrapped = false; let escaped = null; @@ -229,6 +233,8 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { return; } + hasCompletions = true; + // If there is a common prefix to all matches, then apply that portion. const completions = rawCompletions.filter((e) => e); const prefix = commonPrefix(completions); @@ -292,8 +298,11 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { // may be inspected. } else if (preview.exceptionDetails && (result.className === 'EvalError' || - result.className === 'SyntaxError' || - result.className === 'ReferenceError')) { + result.className === 'SyntaxError' || + // Report ReferenceError in case the strict mode is active + // for input that has no completions. + (result.className === 'ReferenceError' && + (hasCompletions || repl.replMode !== REPL_MODE_STRICT)))) { callback(null, null); } else if (result.objectId) { // The writer options might change and have influence on the inspect @@ -339,6 +348,8 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { return; } + hasCompletions = false; + // Add the autocompletion preview. const insertPreview = false; showCompletionPreview(repl.line, insertPreview); @@ -703,6 +714,8 @@ function setupReverseSearch(repl) { } module.exports = { + REPL_MODE_SLOPPY: Symbol('repl-sloppy'), + REPL_MODE_STRICT, isRecoverableError, kStandaloneREPL: Symbol('kStandaloneREPL'), setupPreview, diff --git a/lib/repl.js b/lib/repl.js index 0f00632d35d515..5f6d904ec65e0a 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -103,6 +103,8 @@ const experimentalREPLAwait = require('internal/options').getOptionValue( '--experimental-repl-await' ); const { + REPL_MODE_SLOPPY, + REPL_MODE_STRICT, isRecoverableError, kStandaloneREPL, setupPreview, @@ -896,8 +898,8 @@ ObjectSetPrototypeOf(REPLServer, Interface); exports.REPLServer = REPLServer; -exports.REPL_MODE_SLOPPY = Symbol('repl-sloppy'); -exports.REPL_MODE_STRICT = Symbol('repl-strict'); +exports.REPL_MODE_SLOPPY = REPL_MODE_SLOPPY; +exports.REPL_MODE_STRICT = REPL_MODE_STRICT; // Prompt is a string to print on each line for the prompt, // source is a stream to use for I/O, defaulting to stdin/stdout. diff --git a/test/parallel/test-repl-mode.js b/test/parallel/test-repl-mode.js index df84d86a3c8dc3..514fbbbbe3f8b5 100644 --- a/test/parallel/test-repl-mode.js +++ b/test/parallel/test-repl-mode.js @@ -7,7 +7,8 @@ const repl = require('repl'); const tests = [ testSloppyMode, testStrictMode, - testAutoMode + testAutoMode, + testStrictModeTerminal, ]; tests.forEach(function(test) { @@ -37,6 +38,18 @@ function testStrictMode() { assert.strictEqual(cli.output.accumulator.join(''), 'undefined\n> '); } +function testStrictModeTerminal() { + // Verify that ReferenceErrors are reported in strict mode previews. + const cli = initRepl(repl.REPL_MODE_STRICT, { + terminal: true + }); + + cli.input.emit('data', 'xyz '); + assert.ok( + cli.output.accumulator.includes('\n// ReferenceError: xyz is not defined') + ); +} + function testAutoMode() { const cli = initRepl(repl.REPL_MODE_MAGIC); @@ -48,7 +61,7 @@ function testAutoMode() { assert.strictEqual(cli.output.accumulator.join(''), 'undefined\n> '); } -function initRepl(mode) { +function initRepl(mode, options) { const input = new Stream(); input.write = input.pause = input.resume = () => {}; input.readable = true; @@ -65,6 +78,7 @@ function initRepl(mode) { output: output, useColors: false, terminal: false, - replMode: mode + replMode: mode, + ...options }); } From a62e2b226eca0b0a7a01d79a38dbe1f2d7a08b4d Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 7 May 2020 17:09:51 +0200 Subject: [PATCH 3/8] repl: replace hard coded core module list with actual list This replaces the internally used hard coded Node.js core module list with the actual internal existent modules. That way all modules are automatically picked up instead of having to update the list manually. This currently only applies to the REPL and to the Node.js `eval` functionality (User passed `-e` or `--eval` arguments to Node without `-i` or `--interactive`). Signed-off-by: Ruben Bridgewater --- lib/internal/modules/cjs/helpers.js | 58 ++++-------------------- lib/repl.js | 5 +- test/parallel/test-bootstrap-modules.js | 2 +- test/parallel/test-module-cjs-helpers.js | 9 ---- test/parallel/test-repl-mode.js | 4 ++ test/parallel/test-repl-tab-complete.js | 6 ++- 6 files changed, 22 insertions(+), 62 deletions(-) delete mode 100644 test/parallel/test-module-cjs-helpers.js diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 2f62c8623e27be..2181fc2a42cab3 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -2,6 +2,7 @@ const { ObjectDefineProperty, + ObjectPrototypeHasOwnProperty, SafeMap, } = primordials; const { @@ -109,55 +110,17 @@ function stripBOM(content) { return content; } -const builtinLibs = [ - 'assert', - 'async_hooks', - 'buffer', - 'child_process', - 'cluster', - 'crypto', - 'dgram', - 'dns', - 'domain', - 'events', - 'fs', - 'http', - 'http2', - 'https', - 'net', - 'os', - 'path', - 'perf_hooks', - 'punycode', - 'querystring', - 'readline', - 'repl', - 'stream', - 'string_decoder', - 'tls', - 'trace_events', - 'tty', - 'url', - 'util', - 'v8', - 'vm', - 'worker_threads', - 'zlib', -]; - -if (internalBinding('config').experimentalWasi) { - builtinLibs.push('wasi'); - builtinLibs.sort(); -} - -if (typeof internalBinding('inspector').open === 'function') { - builtinLibs.push('inspector'); - builtinLibs.sort(); -} - function addBuiltinLibsToObject(object) { // Make built-in modules available directly (loaded lazily). - builtinLibs.forEach((name) => { + const { builtinModules } = require('internal/modules/cjs/loader').Module; + builtinModules.forEach((name) => { + // Neither add underscored modules, nor ones that contain slashes (e.g., + // 'fs/promises') or ones that are already defined. + if (name.startsWith('_') || + name.includes('/') || + ObjectPrototypeHasOwnProperty(object, name)) { + return; + } // Goals of this mechanism are: // - Lazy loading of built-in modules // - Having all built-in modules available as non-enumerable properties @@ -203,7 +166,6 @@ function normalizeReferrerURL(referrer) { module.exports = { addBuiltinLibsToObject, - builtinLibs, loadNativeModule, makeRequireFunction, normalizeReferrerURL, diff --git a/lib/repl.js b/lib/repl.js index 5f6d904ec65e0a..9895f563a95bd7 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -63,7 +63,6 @@ const { } = primordials; const { - builtinLibs, makeRequireFunction, addBuiltinLibsToObject } = require('internal/modules/cjs/helpers'); @@ -86,6 +85,8 @@ const { } = require('internal/readline/utils'); const { Console } = require('console'); const CJSModule = require('internal/modules/cjs/loader').Module; +const builtinModules = [...CJSModule.builtinModules] + .filter((e) => !e.startsWith('_')); const domain = require('domain'); const debug = require('internal/util/debuglog').debuglog('repl'); const { @@ -158,7 +159,7 @@ module.paths = CJSModule._nodeModulePaths(module.filename); const writer = exports.writer = (obj) => inspect(obj, writer.options); writer.options = { ...inspect.defaultOptions, showProxy: true }; -exports._builtinLibs = builtinLibs; +exports._builtinLibs = builtinModules; function REPLServer(prompt, stream, diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index e07ca03e807b5d..f28dceeaa10ed1 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -17,7 +17,6 @@ const expectedModules = new Set([ 'Internal Binding credentials', 'Internal Binding fs', 'Internal Binding fs_dir', - 'Internal Binding inspector', 'Internal Binding module_wrap', 'Internal Binding native_module', 'Internal Binding options', @@ -115,6 +114,7 @@ if (common.hasIntl) { } if (process.features.inspector) { + expectedModules.add('Internal Binding inspector'); expectedModules.add('NativeModule internal/inspector_async_hook'); expectedModules.add('NativeModule internal/util/inspector'); } diff --git a/test/parallel/test-module-cjs-helpers.js b/test/parallel/test-module-cjs-helpers.js deleted file mode 100644 index 12de65598e54e1..00000000000000 --- a/test/parallel/test-module-cjs-helpers.js +++ /dev/null @@ -1,9 +0,0 @@ -'use strict'; -// Flags: --expose-internals - -require('../common'); -const assert = require('assert'); -const { builtinLibs } = require('internal/modules/cjs/helpers'); - -const expectedLibs = process.features.inspector ? 34 : 33; -assert.strictEqual(builtinLibs.length, expectedLibs); diff --git a/test/parallel/test-repl-mode.js b/test/parallel/test-repl-mode.js index 514fbbbbe3f8b5..d8131d34f93b44 100644 --- a/test/parallel/test-repl-mode.js +++ b/test/parallel/test-repl-mode.js @@ -39,6 +39,10 @@ function testStrictMode() { } function testStrictModeTerminal() { + if (!process.features.inspector) { + console.warn('Test skipped: V8 inspector is disabled'); + return; + } // Verify that ReferenceErrors are reported in strict mode previews. const cli = initRepl(repl.REPL_MODE_STRICT, { terminal: true diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index 6cf689c4b11074..584dbc21cf4a49 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -30,6 +30,7 @@ const { const assert = require('assert'); const path = require('path'); const fixtures = require('../common/fixtures'); +const { builtinModules } = require('module'); const hasInspector = process.features.inspector; if (!common.isMainThread) @@ -222,8 +223,9 @@ putIn.run(['.clear']); testMe.complete('require(\'', common.mustCall(function(error, data) { assert.strictEqual(error, null); - repl._builtinLibs.forEach(function(lib) { - assert(data[0].includes(lib), `${lib} not found`); + builtinModules.forEach((lib) => { + if (!lib.startsWith('_')) + assert(data[0].includes(lib), `${lib} not found`); }); })); From c24445d21e7b9854995ef41986397d48cb9aec24 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 7 May 2020 20:58:50 +0200 Subject: [PATCH 4/8] test: refactor test/parallel/test-bootstrap-modules.js This simplifies the test a bit by removing duplicated code and by focusing the reader on the passed through module. Signed-off-by: Ruben Bridgewater --- test/parallel/test-bootstrap-modules.js | 42 +++++++++++++------------ 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index f28dceeaa10ed1..ea8eee1553219b 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -85,26 +85,28 @@ const expectedModules = new Set([ ]); if (!common.isMainThread) { - expectedModules.add('Internal Binding messaging'); - expectedModules.add('Internal Binding symbols'); - expectedModules.add('Internal Binding worker'); - expectedModules.add('NativeModule _stream_duplex'); - expectedModules.add('NativeModule _stream_passthrough'); - expectedModules.add('NativeModule _stream_readable'); - expectedModules.add('NativeModule _stream_transform'); - expectedModules.add('NativeModule _stream_writable'); - expectedModules.add('NativeModule internal/error-serdes'); - expectedModules.add('NativeModule internal/process/worker_thread_only'); - expectedModules.add('NativeModule internal/streams/buffer_list'); - expectedModules.add('NativeModule internal/streams/destroy'); - expectedModules.add('NativeModule internal/streams/end-of-stream'); - expectedModules.add('NativeModule internal/streams/legacy'); - expectedModules.add('NativeModule internal/streams/pipeline'); - expectedModules.add('NativeModule internal/streams/state'); - expectedModules.add('NativeModule internal/worker'); - expectedModules.add('NativeModule internal/worker/io'); - expectedModules.add('NativeModule stream'); - expectedModules.add('NativeModule worker_threads'); + [ + 'Internal Binding messaging', + 'Internal Binding symbols', + 'Internal Binding worker', + 'NativeModule _stream_duplex', + 'NativeModule _stream_passthrough', + 'NativeModule _stream_readable', + 'NativeModule _stream_transform', + 'NativeModule _stream_writable', + 'NativeModule internal/error-serdes', + 'NativeModule internal/process/worker_thread_only', + 'NativeModule internal/streams/buffer_list', + 'NativeModule internal/streams/destroy', + 'NativeModule internal/streams/end-of-stream', + 'NativeModule internal/streams/legacy', + 'NativeModule internal/streams/pipeline', + 'NativeModule internal/streams/state', + 'NativeModule internal/worker', + 'NativeModule internal/worker/io', + 'NativeModule stream', + 'NativeModule worker_threads', + ].forEach(expectedModules.add.bind(expectedModules)); } if (common.hasIntl) { From 00cd4d83db48d2201fcf87b96c7af77e966d2725 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 7 May 2020 17:13:07 +0200 Subject: [PATCH 5/8] repl: improve repl autocompletion for require calls This improves the autocompletion for require calls. It had multiple small issues so far. Most important: it won't suggest completions for require statements that are fully written out. Second, it'll detect require calls that have whitespace behind the opening bracket. Third, it makes sure node modules are detected as such instead of only suggesting them as folders. Last, it adds suggestions for input that starts with backticks. Fixes: https://github.com/nodejs/node/issues/33238 Signed-off-by: Ruben Bridgewater --- lib/repl.js | 85 ++++++++----------- .../node_modules/no_index/lib/index.js | 1 + .../node_modules/no_index/package.json | 3 + test/parallel/test-repl-tab-complete.js | 36 ++++++-- 4 files changed, 70 insertions(+), 55 deletions(-) create mode 100644 test/fixtures/node_modules/no_index/lib/index.js create mode 100644 test/fixtures/node_modules/no_index/package.json diff --git a/lib/repl.js b/lib/repl.js index 9895f563a95bd7..084d876d770ad6 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -1047,7 +1047,7 @@ REPLServer.prototype.turnOffEditorMode = deprecate( 'REPLServer.turnOffEditorMode() is deprecated', 'DEP0078'); -const requireRE = /\brequire\s*\(['"](([\w@./-]+\/)?(?:[\w@./-]*))/; +const requireRE = /\brequire\s*\(\s*['"`](([\w@./-]+\/)?(?:[\w@./-]*))(?![^'"`])$/; const fsAutoCompleteRE = /fs(?:\.promises)?\.\s*[a-z][a-zA-Z]+\(\s*["'](.*)/; const simpleExpressionRE = /(?:[a-zA-Z_$](?:\w|\$)*\.)*[a-zA-Z_$](?:\w|\$)*\.?$/; @@ -1095,8 +1095,13 @@ REPLServer.prototype.complete = function() { this.completer.apply(this, arguments); }; -// TODO: Native module names should be auto-resolved. -// That improves the auto completion. +function gracefulOperation(fn, args, alternative) { + try { + return fn(...args); + } catch { + return alternative; + } +} // Provide a list of completions for the given leading text. This is // given to the readline interface for handling tab completion. @@ -1118,26 +1123,25 @@ function complete(line, callback) { // REPL commands (e.g. ".break"). let filter; - let match = line.match(/^\s*\.(\w*)$/); - if (match) { + if (/^\s*\.(\w*)$/.test(line)) { completionGroups.push(ObjectKeys(this.commands)); - completeOn = match[1]; - if (match[1].length) { - filter = match[1]; + completeOn = line.match(/^\s*\.(\w*)$/)[1]; + if (completeOn.length) { + filter = completeOn; } completionGroupsLoaded(); - } else if (match = line.match(requireRE)) { + } else if (requireRE.test(line)) { // require('...') - const exts = ObjectKeys(this.context.require.extensions); - const indexRe = new RegExp('^index(?:' + exts.map(regexpEscape).join('|') + - ')$'); + const extensions = ObjectKeys(this.context.require.extensions); + const indexes = extensions.map((extension) => `index${extension}`); + indexes.push('package.json', 'index'); const versionedFileNamesRe = /-\d+\.\d+/; + const match = line.match(requireRE); completeOn = match[1]; const subdir = match[2] || ''; - filter = match[1]; - let dir, files, subfiles, isDirectory; + filter = completeOn; group = []; let paths = []; @@ -1151,15 +1155,10 @@ function complete(line, callback) { paths = module.paths.concat(CJSModule.globalPaths); } - for (let i = 0; i < paths.length; i++) { - dir = path.resolve(paths[i], subdir); - try { - files = fs.readdirSync(dir); - } catch { - continue; - } - for (let f = 0; f < files.length; f++) { - const name = files[f]; + for (let dir of paths) { + dir = path.resolve(dir, subdir); + const files = gracefulOperation(fs.readdirSync, [dir], []); + for (const name of files) { const ext = path.extname(name); const base = name.slice(0, -ext.length); if (versionedFileNamesRe.test(base) || name === '.npm') { @@ -1167,25 +1166,20 @@ function complete(line, callback) { continue; } const abs = path.resolve(dir, name); - try { - isDirectory = fs.statSync(abs).isDirectory(); - } catch { + const stats = gracefulOperation(fs.statSync, [abs]); + if (!stats || !stats.isDirectory()) { + if (extensions.includes(ext) && (!subdir || base !== 'index')) { + group.push(`${subdir}${base}`); + } continue; } - if (isDirectory) { - group.push(subdir + name + '/'); - try { - subfiles = fs.readdirSync(abs); - } catch { - continue; - } - for (let s = 0; s < subfiles.length; s++) { - if (indexRe.test(subfiles[s])) { - group.push(subdir + name); - } + group.push(`${subdir}${name}/`); + const subfiles = gracefulOperation(fs.readdirSync, [abs], []); + for (const subfile of subfiles) { + if (indexes.includes(subfile)) { + group.push(`${subdir}${name}`); + break; } - } else if (exts.includes(ext) && (!subdir || base !== 'index')) { - group.push(subdir + base); } } } @@ -1198,11 +1192,10 @@ function complete(line, callback) { } completionGroupsLoaded(); - } else if (match = line.match(fsAutoCompleteRE)) { - - let filePath = match[1]; - let fileList; + } else if (fsAutoCompleteRE.test(line)) { filter = ''; + let filePath = line.match(fsAutoCompleteRE)[1]; + let fileList; try { fileList = fs.readdirSync(filePath, { withFileTypes: true }); @@ -1233,7 +1226,7 @@ function complete(line, callback) { // foo<|> # all scope vars with filter 'foo' // foo.<|> # completions for 'foo' with filter '' } else if (line.length === 0 || /\w|\.|\$/.test(line[line.length - 1])) { - match = simpleExpressionRE.exec(line); + const match = simpleExpressionRE.exec(line); if (line.length !== 0 && !match) { completionGroupsLoaded(); return; @@ -1583,10 +1576,6 @@ function defineDefaultCommands(repl) { } } -function regexpEscape(s) { - return s.replace(/[-[\]{}()*+?.,\\^$|#\s]/g, '\\$&'); -} - function Recoverable(err) { this.err = err; } diff --git a/test/fixtures/node_modules/no_index/lib/index.js b/test/fixtures/node_modules/no_index/lib/index.js new file mode 100644 index 00000000000000..4ba52ba2c8df67 --- /dev/null +++ b/test/fixtures/node_modules/no_index/lib/index.js @@ -0,0 +1 @@ +module.exports = {} diff --git a/test/fixtures/node_modules/no_index/package.json b/test/fixtures/node_modules/no_index/package.json new file mode 100644 index 00000000000000..0c6100eadad184 --- /dev/null +++ b/test/fixtures/node_modules/no_index/package.json @@ -0,0 +1,3 @@ +{ + "main": "./lib/index.js" +} diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index 584dbc21cf4a49..c40485ce3f5eac 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -229,24 +229,46 @@ testMe.complete('require(\'', common.mustCall(function(error, data) { }); })); -testMe.complete('require(\'n', common.mustCall(function(error, data) { +testMe.complete("require\t( 'n", common.mustCall(function(error, data) { assert.strictEqual(error, null); assert.strictEqual(data.length, 2); assert.strictEqual(data[1], 'n'); - assert(data[0].includes('net')); + // There is only one Node.js module that starts with n: + assert.strictEqual(data[0][0], 'net'); + assert.strictEqual(data[0][1], ''); // It's possible to pick up non-core modules too - data[0].forEach(function(completion) { - if (completion) - assert(/^n/.test(completion)); + data[0].slice(2).forEach((completion) => { + assert.match(completion, /^n/); }); })); { const expected = ['@nodejsscope', '@nodejsscope/']; + // Require calls should handle all types of quotation marks. + for (const quotationMark of ["'", '"', '`']) { + putIn.run(['.clear']); + testMe.complete('require(`@nodejs', common.mustCall((err, data) => { + assert.strictEqual(err, null); + assert.deepStrictEqual(data, [expected, '@nodejs']); + })); + + putIn.run(['.clear']); + // Completions should not be greedy in case the quotation ends. + const input = `require(${quotationMark}@nodejsscope${quotationMark}`; + testMe.complete(input, common.mustCall((err, data) => { + assert.strictEqual(err, null); + assert.deepStrictEqual(data, [[], undefined]); + })); + } +} + +{ putIn.run(['.clear']); - testMe.complete('require(\'@nodejs', common.mustCall((err, data) => { + // Completions should find modules and handle whitespace after the opening + // bracket. + testMe.complete('require \t("no_ind', common.mustCall((err, data) => { assert.strictEqual(err, null); - assert.deepStrictEqual(data, [expected, '@nodejs']); + assert.deepStrictEqual(data, [['no_index', 'no_index/'], 'no_ind']); })); } From abc2bdabbe8fbf389f0d0a99913bae6f47467944 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 11 May 2020 11:44:14 +0200 Subject: [PATCH 6/8] fixup! repl: show reference errors during preview This adds support for `--use-strict`. Signed-off-by: Ruben Bridgewater --- lib/internal/repl/utils.js | 8 +++- lib/repl.js | 3 +- .../test-repl-strict-mode-previews.js | 44 +++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-repl-strict-mode-previews.js diff --git a/lib/internal/repl/utils.js b/lib/internal/repl/utils.js index 0a79fb44babf92..160417491fe804 100644 --- a/lib/internal/repl/utils.js +++ b/lib/internal/repl/utils.js @@ -271,6 +271,12 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { }); } + function isInStrictMode(repl) { + return repl.replMode === REPL_MODE_STRICT || process.execArgv + .map((e) => e.toLowerCase().replace(/_/g, '-')) + .includes('--use-strict'); + } + // This returns a code preview for arbitrary input code. function getInputPreview(input, callback) { // For similar reasons as `defaultEval`, wrap expressions starting with a @@ -302,7 +308,7 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { // Report ReferenceError in case the strict mode is active // for input that has no completions. (result.className === 'ReferenceError' && - (hasCompletions || repl.replMode !== REPL_MODE_STRICT)))) { + (hasCompletions || !isInStrictMode(repl))))) { callback(null, null); } else if (result.objectId) { // The writer options might change and have influence on the inspect diff --git a/lib/repl.js b/lib/repl.js index 084d876d770ad6..595f41cf005ff9 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -366,8 +366,7 @@ function REPLServer(prompt, } while (true) { try { - if (!/^\s*$/.test(code) && - self.replMode === exports.REPL_MODE_STRICT) { + if (self.replMode === exports.REPL_MODE_STRICT && !/^\s*$/.test(code)) { // "void 0" keeps the repl from returning "use strict" as the result // value for statements and declarations that don't return a value. code = `'use strict'; void 0;\n${code}`; diff --git a/test/parallel/test-repl-strict-mode-previews.js b/test/parallel/test-repl-strict-mode-previews.js new file mode 100644 index 00000000000000..2c3e8d5b938d77 --- /dev/null +++ b/test/parallel/test-repl-strict-mode-previews.js @@ -0,0 +1,44 @@ +// Previews in strict mode should indicate ReferenceErrors. + +'use strict'; + +require('../common'); + +if (process.argv[2] === 'child') { + const stream = require('stream'); + const repl = require('repl'); + class ActionStream extends stream.Stream { + readable = true; + run(data) { + this.emit('data', `${data}`); + this.emit('keypress', '', { ctrl: true, name: 'd' }); + } + resume() {} + pause() {} + } + + repl.start({ + input: new ActionStream(), + output: new stream.Writable({ + write(chunk, _, next) { + console.log(chunk.toString()); + next(); + } + }), + useColors: false, + terminal: true + }).inputStream.run('xyz'); +} else { + const assert = require('assert'); + const { spawnSync } = require('child_process'); + + const result = spawnSync( + process.execPath, + ['--use-strict', `${__filename}`, 'child'] + ); + + assert.match( + result.stdout.toString(), + /\/\/ ReferenceError: xyz is not defined/ + ); +} From 2e04c5d471ae0f7a24f6f66cc8301bfb50f4f2f6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 May 2020 18:02:39 +0200 Subject: [PATCH 7/8] fixup! repl: show reference errors during preview Signed-off-by: Ruben Bridgewater --- test/parallel/test-repl-strict-mode-previews.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-repl-strict-mode-previews.js b/test/parallel/test-repl-strict-mode-previews.js index 2c3e8d5b938d77..8da4029e186b3c 100644 --- a/test/parallel/test-repl-strict-mode-previews.js +++ b/test/parallel/test-repl-strict-mode-previews.js @@ -2,7 +2,9 @@ 'use strict'; -require('../common'); +const common = require('../common'); + +common.skipIfInspectorDisabled(); if (process.argv[2] === 'child') { const stream = require('stream'); From e8294cf1aa07c2197d03ccb823567c0a815e2031 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 May 2020 18:05:15 +0200 Subject: [PATCH 8/8] fixup! repl: improve repl autocompletion for require calls Simplify implementation and reduce file system calls. Signed-off-by: Ruben Bridgewater --- lib/repl.js | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index 595f41cf005ff9..bd7b80a91b57ea 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -1156,27 +1156,30 @@ function complete(line, callback) { for (let dir of paths) { dir = path.resolve(dir, subdir); - const files = gracefulOperation(fs.readdirSync, [dir], []); - for (const name of files) { - const ext = path.extname(name); - const base = name.slice(0, -ext.length); - if (versionedFileNamesRe.test(base) || name === '.npm') { + const dirents = gracefulOperation( + fs.readdirSync, + [dir, { withFileTypes: true }], + [] + ); + for (const dirent of dirents) { + if (versionedFileNamesRe.test(dirent.name) || dirent.name === '.npm') { // Exclude versioned names that 'npm' installs. continue; } - const abs = path.resolve(dir, name); - const stats = gracefulOperation(fs.statSync, [abs]); - if (!stats || !stats.isDirectory()) { - if (extensions.includes(ext) && (!subdir || base !== 'index')) { + const extension = path.extname(dirent.name); + const base = dirent.name.slice(0, -extension.length); + if (!dirent.isDirectory()) { + if (extensions.includes(extension) && (!subdir || base !== 'index')) { group.push(`${subdir}${base}`); } continue; } - group.push(`${subdir}${name}/`); - const subfiles = gracefulOperation(fs.readdirSync, [abs], []); + group.push(`${subdir}${dirent.name}/`); + const absolute = path.resolve(dir, dirent.name); + const subfiles = gracefulOperation(fs.readdirSync, [absolute], []); for (const subfile of subfiles) { if (indexes.includes(subfile)) { - group.push(`${subdir}${name}`); + group.push(`${subdir}${dirent.name}`); break; } }