feat: replace Node.js REPL with plain vm context for script usage MONGOSH-1720 #1849
Conversation
…GOSH-1720 - Allow running `MongoshNodeRepl` instances either based on: - The existing mechanism for running code, which is spinning up a Node.js REPL and using it to evaluate code; or - A more lightweight mechanism that only creates a `vm` context and then run code using that context directly. - Introduce a new command-line switch, `--jsContext`, that can be used to explicitly select the desired behavior, with possible values of `repl`, `plain-vm` and `auto`. The default behavior is to switch depending on whether the CLI will enter interactive mode or not. Running in `plain-vm` mode will significantly improve runtime performance (a 6× reduction of script run time in local testing), coming from the removal of async context tracking that the REPL uses to identify async operations which were originally spawned from the REPL instance in question. This lack of async context tracking comes with some implications, in particular asynchronously thrown errors (in the Node.js sense, e.g. `setImmediate(() => { throw ... })`) will lead to slightly different behavior (e.g. different error code and error output). That is probably acceptable breakage in this context.
addaleax
changed the title
| // for debugging | ||
| if (process.env.MONGOSH_DISPLAY_STARTUP_STACK_TRACE) | ||
| console.error(e?.stack); | ||
| else console.error(`${e?.name}: ${e?.message}`); |
There was a problem hiding this comment.
This is just for us, it's pretty annoying to have the stack trace not showing up for errors occuring during startup
There was a problem hiding this comment.
I should probably already know this answer, but why don't we display the stack trace by default all the time?
There was a problem hiding this comment.
The original idea here was that mongosh wouldn't display stack traces because they often do not contain relevant information for users. Maybe this is something we can revisit after https://jira.mongodb.org/browse/NODE-5556 where the driver team aims to implement proper async stack traces.
| @@ -160,7 +160,7 @@ export class TestShell { | |||
| return this._rawOutput; | |||
| } | |||
|
|
|||
| get process(): ChildProcess { | |||
| get process(): ChildProcessWithoutNullStreams { | |||
There was a problem hiding this comment.
Just a drive-by to fix type errors showing up in the e2e.spec.ts file
|
Why is this not versioned as a breaking change? This breaks javascript file executions that uses native things like |
|
Yes, this should be versioned as a breaking change, I have several scripts impacted with these changes |
|
And the helper doesn't show up the new |
|
@MarcusElevait @tiagoliveira9 Just for clarity, this was unintended breakage and we released mongosh 2.2.1 soon after to fix it: https://github.com/mongodb-js/mongosh/releases/tag/v2.2.1 |
[Definitely recommend viewing the diff in whitespace-changes-disabled mode :)]
MongoshNodeReplinstances either based on:a Node.js REPL and using it to evaluate code; or
vmcontextand then run code using that context directly.
--jsContext, that can beused to explicitly select the desired behavior, with possible values
of
repl,plain-vmandauto. The default behavior is to switchdepending on whether the CLI will enter interactive mode or not.
Running in
plain-vmmode will significantly improve runtimeperformance (a 6× reduction of script run time in local testing),
coming from the removal of async context tracking that the REPL
uses to identify async operations which were originally spawned
from the REPL instance in question.
This lack of async context tracking comes with some implications,
in particular asynchronously thrown errors (in the Node.js sense,
e.g.
setImmediate(() => { throw ... })) will lead to slightlydifferent behavior (e.g. different error code and error output).
That is probably acceptable breakage in this context.