-
Notifications
You must be signed in to change notification settings - Fork 77
feat(node-runtime-worker-thread): Add new runtime that works in the worker thread COMPASS-4555 #533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rerequisite for inlining workers
…ferring to it by the path
…void breaking shell api in prod build
…e hopes of fixing windows build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @addaleax wants to give a look too, but it looks amazing to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! :)
this.initWorkerPromise = this.initWorker(); | ||
} | ||
|
||
private async initWorker() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I’m a bit surprised that the linter is okay with leaving out return value descriptions… I thought it complained about that for other packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I can't see the rule enabled in our eslint config, and it's not a recommended one 🤔 Should we add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong feelings about that, but I personally like being explicit about the signature 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like the rule too and (as it's pretty obvious from this PR, haha) always forget to add explicit return types when the rule is not there 😄
…of if statement, fix typos Co-authored-by: Anna Henningsen <[email protected]>
…rocess to terminate before resolving Co-authored-by: Anna Henningsen <[email protected]>
…runtime-worker-thread
… tests; Eval worker to mimic real usage better
2413650
to
b51f847
Compare
…in hopes that this will fix win tests
17c9c9c
to
c12e90b
Compare
5683a88
to
45f26dc
Compare
07d5e33
to
d5aa34c
Compare
This PR introduces new mongosh runtime that works in the worker thread and communicates with the main thread through an rpc that matches generic
Runtime
interfaceTo avoid making this PR even bigger we decided to split the work on the runtime in the multiple parts:
ShellResult
s back to the main threadShellResult
cases like primitive values, cursors and errorsmessageBus
integration for telemetrycompass-shell
This PR only addresses the first step: basic implementation. It's mostly based on the PoC (#483) and follows tech design decisions pretty closely if you want additional context on some of the decisions made
Please don't be scared by the line count, its mostly due to the new package being set up and files like package-lock added to the repository. The PR is still pretty massive so sorry for that and I'm of course happy to go through all the changes together