repl: add inline syntax highlighting#64609
Conversation
avivkeller
left a comment
There was a problem hiding this comment.
I have a few concerns here.
- Node provides
util.inspect.styleswhich defines colors, so we don't need to define them separately - This involves hooking into and monkey patching REPL internals. Rather than monkey patching them, they should be adapted to work with new code.
- This is a lot of code to maintain, and it'll require manual editing every time a new keyword is added to V8. AST parsing provides some of that info for us, and we can probably reduce overhead by relying on it.
(Note that I have an alternate implementation at #64591. That PR and this one being opened around the same time is a coincidence of the REPL inspector landing)
b39fe8f to
756e16e
Compare
For context: this is a split from #64443 which predates #64591. Will let you all decide which PR to pick π€ PTAL @avivkeller |
| const { stylizeWithColor } = require('internal/util/inspect'); | ||
| const { Parser } = require('internal/deps/acorn/acorn/dist/acorn'); | ||
|
|
||
| const tokenizer = FunctionPrototypeBind(Parser.tokenizer, Parser); | ||
|
|
||
| /** | ||
| * Maps an Acorn token to a util.inspect style name. | ||
| * Returns undefined for tokens that should not be highlighted. | ||
| * @param {object} token The Acorn token. | ||
| * @returns {string|undefined} The inspect style name. | ||
| */ | ||
| function tokenStyle(token) { | ||
| const { label, keyword } = token.type; | ||
|
|
||
| if (keyword !== undefined) { | ||
| if (label === 'true' || label === 'false') return 'boolean'; | ||
| if (label === 'null') return 'null'; | ||
| if (label === 'import' || label === 'export') return 'module'; | ||
| return 'special'; | ||
| } | ||
|
|
||
| switch (label) { | ||
| case 'name': | ||
| switch (token.value) { | ||
| case 'undefined': | ||
| return 'undefined'; | ||
| case 'NaN': | ||
| case 'Infinity': | ||
| return 'number'; | ||
| default: | ||
| return undefined; | ||
| } | ||
| case 'num': | ||
| return 'number'; | ||
| case 'bigint': | ||
| return 'bigint'; | ||
| case 'string': | ||
| case 'template': | ||
| case '`': | ||
| return 'string'; | ||
| case 'regexp': | ||
| return 'regexp'; | ||
| default: | ||
| return undefined; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Applies ANSI syntax highlighting to a JavaScript code string. | ||
| * Uses the Acorn tokenizer and util.inspect.styles for colors, | ||
| * so the color scheme stays in sync with util.inspect output. | ||
| * @param {string} code The JavaScript code string to highlight. | ||
| * @returns {string} The highlighted code string. | ||
| */ | ||
| function syntaxHighlight(code) { | ||
| if (code.length === 0) return code; | ||
|
|
||
| let result = ''; | ||
| let offset = 0; | ||
|
|
||
| function write(start, end, inspectStyle) { | ||
| result += | ||
| StringPrototypeSlice(code, offset, start) + | ||
| stylizeWithColor(StringPrototypeSlice(code, start, end), inspectStyle); | ||
| offset = end; | ||
| } | ||
|
|
||
| try { | ||
| const iterator = tokenizer(code, { | ||
| __proto__: null, | ||
| allowHashBang: true, | ||
| ecmaVersion: 'latest', | ||
| onComment(_block, _text, start, end) { | ||
| write(start, end, 'undefined'); | ||
| }, | ||
| }); | ||
|
|
||
| for (const token of iterator) { | ||
| const inspectStyle = tokenStyle(token); | ||
| if (inspectStyle !== undefined) { | ||
| write(token.start, token.end, inspectStyle); | ||
| } | ||
| } | ||
| } catch { | ||
| // Acorn throws for unfinished strings, templates, and comments. | ||
| // Tokens reported before the error are still useful while typing. | ||
| } | ||
|
|
||
| return result + StringPrototypeSlice(code, offset); | ||
| } | ||
|
|
There was a problem hiding this comment.
While I respect that you changed this following my review, this code is plagiarized from my own. It's important to see your own spin on things.
There was a problem hiding this comment.
Hey @avivkeller β "plagiarized" is a strong word. The original implementation was in #64443 which you reviewed and suggested splitting into separate PRs. While I was working on that split, you opened #64591 independently. When your review on this PR suggested using stylizeWithColor and the colorize hook, I adopted those patterns from your PR to address your feedback β that felt like the right thing to do.
If the code ended up too close to yours, a co-author credit would have been the collaborative path forward. That said, you are one of the maintainers β feel free to close this PR if you'd prefer to go with #64591.
There was a problem hiding this comment.
I don't think closing this is the right call. My PR doesn't get any special priority just because I'm a maintainer, as we treat all PRs equally, and that includes mine.
That said, regarding your statements:
-
"I adopted those patterns from your PR." Adopting patterns means using existing code as a reference to write something new. What actually happened here is that my changes were inserted into yours almost 1:1 (which is also why the lint is failing).
-
"if the code ended up too close to yours": if you'd referenced my code and it came out similar, that would be completely fine. Referencing each other's work is a normal part of collaboration. Copying it verbatim is a different thing.
There was a problem hiding this comment.
Totally understand, it was a cherry-pick there, as I was addressing your review wanted to use the same patterns you have defined.
What do you suggest we do with PR?
- Do a new implementation of the same challenge?
- Close this on behalf?
- Revert this PR to the pervious state and enhance?
- Any other alternatives.
53b4780 to
405267e
Compare
Use the Acorn tokenizer to colorize JavaScript input in real-time. Keywords are magenta, strings green, numbers yellow, regexps red, and comments gray. Highlighting is display-only β repl.line stays plain text. Can be disabled via `syntaxHighlighting: false` in repl.start() or TERM=dumb. Refs: nodejs#48164 Signed-off-by: hemanth <hemanth.hm@gmail.com>
405267e to
f74ff7b
Compare
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64609 +/- ##
==========================================
- Coverage 90.14% 90.10% -0.04%
==========================================
Files 741 741
Lines 242080 242189 +109
Branches 45563 45572 +9
==========================================
+ Hits 218213 218226 +13
- Misses 15375 15472 +97
+ Partials 8492 8491 -1
π New features to boost your workflow:
|
Split from #64443 per review feedback to use idiomatic single-feature PRs.
Uses the Acorn tokenizer (already in
deps/) to colorize JavaScript input in real-time:Highlighting is display-only β
repl.linestays plain text. Can be disabled viasyntaxHighlighting: falseorTERM=dumb.Dogfoods
util.styleTextper @edsadr's suggestion.Refs: #48164