repl: add function signature hints#64610
Conversation
|
@hemanth every time a PR is opened and closed it pings maintainers. May I suggest verifying the branch is correct before opening a PR? |
b064b71 to
6beba0c
Compare
When typing a function name followed by '(', the REPL now displays
a dimmed hint below the input line showing the function's parameters.
For example, typing 'console.log(' shows:
// console.log(...data)
Uses the inspector to safely extract parameter lists via
Function.prototype.toString(), with throwOnSideEffect to prevent
unintended evaluation.
PR-URL: nodejs#64610
fc38014 to
ae8b308
Compare
When typing a function name followed by '(', the REPL now displays
a dimmed hint below the input line showing the function's parameters.
For example, typing 'console.log(' shows:
// console.log(...data)
Uses the inspector to safely extract parameter lists via
Function.prototype.toString(), with throwOnSideEffect to prevent
unintended evaluation.
PR-URL: nodejs#64610
ae8b308 to
7777029
Compare
|
Multiple PRs, multiple branches! After splitting the larger PR it has caused few side-effects, cleaned up the branch, please review @avivkeller thank you! |
When typing a function name followed by '(', the REPL now displays
a dimmed hint below the input line showing the function's parameters.
For example, typing 'console.log(' shows:
// console.log(...data)
Uses the inspector to safely extract parameter lists via
Function.prototype.toString(), with throwOnSideEffect to prevent
unintended evaluation.
PR-URL: nodejs#64610
7777029 to
be1b29e
Compare
When typing a function name followed by '(', the REPL now displays
a dimmed hint below the input line showing the function's parameters.
For example, typing 'console.log(' shows:
// console.log(...data)
Uses the inspector to safely extract parameter lists via
Function.prototype.toString(), with throwOnSideEffect to prevent
unintended evaluation.
PR-URL: nodejs#64610
be1b29e to
c4307f5
Compare
| _builtinLibs = value; | ||
| } | ||
|
|
||
| const { styleText } = require('util'); |
There was a problem hiding this comment.
This should be imported with everything else
| // Find the function name before the opening parenthesis. | ||
| // Look backwards from cursor for pattern: identifier( | ||
| let parenPos = -1; | ||
| for (let i = cursor - 1; i >= 0; i--) { | ||
| const ch = StringPrototypeCharCodeAt(line, i); | ||
| if (ch === 40) { // '(' | ||
| parenPos = i; | ||
| break; | ||
| } | ||
| // If we hit anything other than whitespace or content after '(', | ||
| // stop looking. | ||
| if (ch !== 32 && ch !== 9) break; // space, tab | ||
| } | ||
|
|
||
| if (parenPos < 0) return; | ||
|
|
||
| // Extract the expression before the '(' - supports simple names | ||
| // (e.g. `foo(`) and dotted access (e.g. `console.log(`). | ||
| // Parenthesized expressions like `(fn)(` are not handled; | ||
| // the hint is silently skipped in that case. | ||
| const nameEnd = parenPos; | ||
| let nameStart = nameEnd; | ||
| for (let i = nameEnd - 1; i >= 0; i--) { | ||
| const ch = StringPrototypeCharCodeAt(line, i); | ||
| // Allow identifier chars and dots for property access. | ||
| if ((ch >= 65 && ch <= 90) || (ch >= 97 && ch <= 122) || | ||
| (ch >= 48 && ch <= 57) || ch === 95 || ch === 36 || ch === 46) { | ||
| nameStart = i; | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
How do we get the expression for auto completion? Can we reuse that logic here?
| throwOnSideEffect: true, | ||
| timeout: 200, | ||
| contextId: repl[contextSymbol], | ||
| }).then((preview) => { |
There was a problem hiding this comment.
Would it be cleaner to make this function asynchronous?
| }).catch((err) => { | ||
| process.emitWarning( | ||
| `Signature hint failed: ${err?.message || err}`, | ||
| 'REPLWarning', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I think not handling the error at all is smarter, if something goes wrong, we should bubble it up so that we know, right?
| let showSignatureHint; | ||
| let clearSignatureHint; | ||
| if (preview && this.terminal && process.features.inspector && | ||
| process.env.TERM !== 'dumb') { | ||
| ({ showSignatureHint, clearSignatureHint } = setupSignatureHint( | ||
| this, | ||
| kContextId, | ||
| )); | ||
| } else { | ||
| showSignatureHint = () => {}; | ||
| clearSignatureHint = () => {}; | ||
| } |
There was a problem hiding this comment.
Rather than setting functions to no-ops, we can use ?.()
| class REPLStream extends Stream { | ||
| readable = true; | ||
| writable = true; | ||
|
|
||
| constructor() { | ||
| super(); | ||
| this.lines = ['']; | ||
| this.writeCount = 0; | ||
| } | ||
| async run(data) { | ||
| for (const entry of data) { | ||
| this.emit('data', entry); | ||
| await this.settle(); | ||
| } | ||
| this.emit('data', '\n'); | ||
| } | ||
| settle() { | ||
| return new Promise((resolve) => { | ||
| let last = this.writeCount; | ||
| let quiet = 0; | ||
| const check = () => { | ||
| if (this.writeCount !== last) { | ||
| last = this.writeCount; | ||
| quiet = 0; | ||
| } else if (++quiet >= 10) { | ||
| resolve(); | ||
| return; | ||
| } | ||
| setImmediate(check); | ||
| }; | ||
| setImmediate(check); | ||
| }); | ||
| } | ||
| write(chunk) { | ||
| this.writeCount++; | ||
| const chunkLines = chunk.toString('utf8').split('\n'); | ||
| this.lines[this.lines.length - 1] += chunkLines[0]; | ||
| if (chunkLines.length > 1) { | ||
| this.lines.push(...chunkLines.slice(1)); | ||
| } | ||
| this.emit('line', this.lines[this.lines.length - 1]); | ||
| return true; | ||
| } | ||
| async wait() { | ||
| this.lines = ['']; | ||
| for await (const [line] of events.on(this, 'line')) { | ||
| if (line.includes(PROMPT)) { | ||
| return this.lines; | ||
| } | ||
| } | ||
| } | ||
| pause() {} | ||
| resume() {} | ||
| } | ||
|
|
||
| async function runAndWait(cmds, repl) { | ||
| const promise = repl.inputStream.wait(); | ||
| for (const cmd of cmds) { | ||
| await repl.inputStream.run(cmd); | ||
| } | ||
| return promise; | ||
| } |
There was a problem hiding this comment.
Does common/repl have something that can do this for us, or is this boilerplate really needed? It seems like more code to maintain
avivkeller
left a comment
There was a problem hiding this comment.
I know I'm leaving a lot of reviews, and I'm very sorry, keep in mind, these are just reviews, and you're not obligated to accept them
| /** | ||
| * Wraps text with an ANSI color via util.styleText. | ||
| * Uses the fast path (validateStream: false) to skip stream checks. | ||
| * @param {string} colorName The color name (e.g. 'magenta', 'green'). | ||
| * @param {string} text The text to colorize. | ||
| * @returns {string} The colorized text. | ||
| */ | ||
| function style(colorName, text) { | ||
| return styleText(colorName, text, kStyleOpts); | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if we can leech off preview and avoid some styling boiler plate?
As in, make this a sub part of preview.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64610 +/- ##
==========================================
- Coverage 90.14% 90.13% -0.01%
==========================================
Files 741 741
Lines 242080 242248 +168
Branches 45563 45593 +30
==========================================
+ Hits 218213 218343 +130
- Misses 15375 15404 +29
- Partials 8492 8501 +9
🚀 New features to boost your workflow:
|
Split from #64443 per review feedback to use idiomatic single-feature PRs.
When the user types
functionName(, the REPL displays the function's parameter list as a dimmed hint below the input line. Uses the V8 Inspector protocol withthrowOnSideEffect: truefor safety.Requires
previewto be enabled and the inspector to be available.Refs: #48164