Skip to content

Conversation

@avivkeller
Copy link
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
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
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
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
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
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