repl: Don’t complete expressions when eval() failed #6328
Conversation
addaleax
force-pushed
the
repl-no-reference-errors
branch
from
33e2402
to
a78221d
Compare
|
I agree about the With 6.0 landing next week, it's a good time to be |
Trott
added
the
semver-major
|
This LGTM but would like to get more review from @nodejs/ctc |
| @@ -806,7 +806,8 @@ REPLServer.prototype.complete = function(line, callback) { | |||
| }); | |||
| } | |||
| } else { | |||
| this.eval(expr, this.context, 'repl', function(e, obj) { | |||
| const evalExpr = `try { ${expr} } catch (e) { undefined }`; | |||
There was a problem hiding this comment.
I don't think the undefined is necessary.
There was a problem hiding this comment.
You’re right. If you want it removed, sure, but I don’t mind the verbosity of writing the “return” value down explicitly.
|
LGTM with a comment. |
addaleax
force-pushed
the
repl-no-reference-errors
branch
from
a78221d
to
ce4ac6e
Compare
|
Rebased & updated with @cjihrig’s suggestion. |
|
@jasnell This one too? I’ll gladly to wait for more LGTMs, but it might be nice to have it in the rc, at least if that’s going to be the last one before the actual release (which I assume it is?). CI looks good. |
|
Let's give this one until Monday just to make sure there are no other objections. @nodejs/ctc ... please take a moment to take a look! |
Instead of changing the way “simple” expressions are detected, switch to ignoring errors when completing. This approach is more generic than the previous one from 0b66b8f, but also changes the way errors are thrown when completing. This reverts the code changes from commit 0b66b8f. The test case is left intact. Fixes: nodejs#6325 PR-URL: nodejs#6328 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax
force-pushed
the
repl-no-reference-errors
branch
from
ce4ac6e
to
05d4103
Compare
|
Squashed & rebased against master, and updated the added tests to use |
|
LGTM after squashing, CI is green. Landing this in a minute |
Instead of changing the way “simple” expressions are detected, switch to ignoring errors when completing. This approach is more generic than the previous one from 0b66b8f, but also changes the way errors are thrown when completing. This reverts the code changes from commit 0b66b8f. The test case is left intact. Fixes: #6325 PR-URL: #6328 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Landed in 3ee68f7 |
Instead of changing the way “simple” expressions are detected, switch to ignoring errors when completing. This approach is more generic than the previous one from 0b66b8f, but also changes the way errors are thrown when completing. This reverts the code changes from commit 0b66b8f. The test case is left intact. Fixes: #6325 PR-URL: #6328 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
Affected core subsystem(s)
repl
Description of change
Instead of changing the way “simple” expressions are detected, switch to ignoring errors when completing. This approach is more generic than the previous one from 0b66b8f, but also changes the way errors are thrown when completing.
Fixes: #6325
This includes a revert of the regular expression change in #6192.
This is also a completely inappropriate solution if it is intended that trying to complete expressions like
a = b.c.dwithbnot defined throw aReferenceError; the test file seems to be laid out to expect that behaviour. I can’t imagine how that might be even remotely desirable, though.