-
Notifications
You must be signed in to change notification settings - Fork 3
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
Use WebWorker #38
Use WebWorker #38
Conversation
@@ -15,9 +16,10 @@ | |||
"resolveJsonModule": true, | |||
"outDir": "lib", | |||
"rootDir": "src", | |||
"skipLibCheck": true, |
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.
Wondering if we should track removing this field at some point, as it usually better practice to not have to use 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.
What change in particular required enabling skipLibCheck
?
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.
Without it I see complaints about @rjsf
:
node_modules/@rjsf/utils/lib/types.d.ts:102:37 - error TS2339: Property 'autocomplete' does not exist on type 'HTMLInputElement'.
102 autoComplete?: HTMLInputElement['autocomplete'];
~~~~~~~~~~~~~~
node_modules/@rjsf/utils/lib/types.d.ts:735:37 - error TS2339: Property 'autocomplete' does not exist on type 'HTMLInputElement'.
735 autocomplete?: HTMLInputElement['autocomplete'];
~~~~~~~~~~~~~~
node_modules/yjs/dist/src/types/YXmlFragment.d.ts:163:36 - error TS2304: Cannot find name 'Node'.
163 } | undefined, binding?: any): Node;
~~~~
I assume it appeared because I added use of JupyterLite DriveFS
and that brings in more dependencies, but I don't know.
I would rather not use the option, but I couldn't find a quick fix.
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.
Opened #41 to track this.
With this PR a shell always runs in a WebWorker rather than the main thread. Up until now the WebWorker code has been in the UI projects (JupyterLite terminal extension and cockle-playground). It makes more sense to have it here so that there is only one copy of it and allows the UI projects to be relatively thin wrappers of cockle. Also, it is important that the shell runs in a WebWorker to allow us to dynamically load WASM commands rather than the current approach of compiling and linking them in the NPM package.
There are now 3 shell-related classes.
Shell
is the external interface to a shell, and runs in the main thread.ShellWorker
runs in a WebWorker and communicates with theShell
viacomlink
and also aSharedArrayBuffer
to provide buffered stdin whilst WASM commands are run.ShellImpl
is the actual shell and is mostly unchanged from the previousShell
class.There are extra
IShell.IOptions
used when creating aShell
, mostly relating to initialising the WASM filesystem. This includes adriveFsBaseUrl
to create a JupyterLiteDriveFS
to access the files displayed in the JupyterLite file browser, and there are alsoinitialDirectories
andinitialFiles
to create which are mostly used for testing.Much of the test suite has been ported, but some tests remain commented out for now. The playwright tests can directly access a
Shell
in the main thread and pass text in and get text output, and they can also access some of the low-level functions liketokenize
andparse
by directly using them without a WebWorker. But accessing other objects of theShellImpl
in the WebWorker is not currently possible, so probably the commented-out tests will eventually be rewritten to access the main threadShell
. It would alternatively be possible to run some units tests in node without a WebWorker, but this has limited use as when the WASM commands can be dynamically loaded, which is intended to be soon, they will only work in a WebWorker.This has been tested against
cockle-playground
but not theterminal
extension as the latter needs larger changes.