Skip to content

repl: print errors in red#52503

Closed
avivkeller wants to merge 4 commits intonodejs:mainfrom
avivkeller:patch-8
Closed

repl: print errors in red#52503
avivkeller wants to merge 4 commits intonodejs:mainfrom
avivkeller:patch-8

Conversation

@avivkeller
Copy link
Copy Markdown
Member

This change will have NodeJS REPLs print errors in red, so that they are easily distinguishable from traditional loggings. This change will not affect console.error

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Apr 12, 2024
@avivkeller
Copy link
Copy Markdown
Member Author

The tests failed due to them not prepared for the color scheme. I can change them once this PR is approved/reveiwed

@lemire
Copy link
Copy Markdown
Member

lemire commented Apr 12, 2024

The tests failed due to them not prepared for the color scheme. I can change them once this PR is approved/reveiwed

I don't know what others think, but I personally think that it would be best to also change the tests.

@avivkeller
Copy link
Copy Markdown
Member Author

The only issue with this (so far) is that when the user throws an object itself, the coloring get's reset after a change:

"\x1B[31mUncaught { foo: \x1B[32m'test'\x1B[39m }\x1B[39m\n"
Uncaught { foo: 'test' }

Imagine that Uncaught { foo: is red, and 'test' } is white

But, the user would have to directly throw an object within the REPL, which almost never happens

@avivkeller
Copy link
Copy Markdown
Member Author

[test-macOS: test/parallel/test-repl-top-level-await.js#L191](https://github.com/nodejs/node/pull/52503/files#annotation_20396691135)
--- stderr ---
node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

  [
    'await Promise..resolve()\r',
...
    '',
    "Unexpected token '.'",
+   '',
    'await repl > '
  ]
    at ordinaryTests (/Users/runner/work/node/node/test/parallel/test-repl-top-level-await.js:191:14)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async main (/Users/runner/work/node/node/test/parallel/test-repl-top-level-await.js:220:3) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: [
    'await Promise..resolve()\r',
    'Uncaught SyntaxError: ',
    'await Promise..resolve()',
    '              ^',
    '',
    "Unexpected token '.'",
    '',
    'await repl > '
  ],
  expected: [
    'await Promise..resolve()\r',
    'Uncaught SyntaxError: ',
    'await Promise..resolve()',
    '              ^',
    '',
    "Unexpected token '.'",
    'await repl > '
  ],
  operator: 'deepStrictEqual'
}

Node.js v22.0.0-pre
--- stdout ---
Testing await Promise.resolve(0)
Testing { a: await Promise.resolve(1) }
Testing _
Testing let { aa, bb } = await Promise.resolve({ aa: 1, bb: 2 }), f = 5;
Testing aa
Testing bb
Testing f
Testing let cc = await Promise.resolve(2)
Testing cc
Testing let dd;
Testing dd
Testing let [ii, { abc: { kk } }] = [0, { abc: { kk: 1 } }];
Testing ii
Testing kk
Testing var ll = await Promise.resolve(2);
Testing ll
Testing foo(await koo())
Testing _
Testing const m = foo(await koo());
Testing m
Testing const n = foo(await
koo());
Testing n
Testing `status: ${(await Promise.resolve({ status: 200 })).status}`
Testing for (let i = 0; i < 2; ++i) await i
Testing for (let i = 0; i < 2; ++i) { await i }
Testing await 0
Testing await 0; function foo() {}
Testing foo
Testing class Foo {}; await 1;
Testing Foo
Testing if (await true) { function bar() {}; }
Testing bar
Testing if (await true) { class Bar {}; }
Testing Bar
Testing await 0; function* gen(){}
Testing for (var i = 0; i < 10; ++i) { await i; }
Testing i
Testing for (let j = 0; j < 5; ++j) { await j; }
Testing j
Testing gen
Testing return 42; await 5;
Testing let o = await 1, p
Testing p
Testing let q = 1, s = await 2
Testing s
Testing for await (let i of [1,2,3]) console.log(i)
Testing await Promise..resolve()
Command: out/Release/node --expose-internals --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /Users/runner/work/node/node/test/parallel/test-repl-top-level-await.js

I'm not quite sure what went wrong here

@avivkeller avivkeller closed this Apr 13, 2024
@avivkeller avivkeller deleted the patch-8 branch April 13, 2024 21:16
@avivkeller
Copy link
Copy Markdown
Member Author

I don't think I'll end up merging this PR, as I'm trying to merge nodejs/repl into the main NodeJS, which will redo all of this anyway.

See #52510 and nodejs/repl#54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants