-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement Custom TS Worker #625
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
camargo
added
the
build
Changes that affect the build system or external dependencies
label
May 8, 2023
camargo
reviewed
May 8, 2023
camargo
reviewed
May 8, 2023
camargo
reviewed
May 8, 2023
camargo
reviewed
May 8, 2023
camargo
reviewed
May 8, 2023
camargo
reviewed
May 8, 2023
camargo
reviewed
May 8, 2023
camargo
force-pushed
the
fix/289/ts_worker_extension
branch
from
May 16, 2023 21:49
339a8fd
to
c9b7284
Compare
Includes a minimal overridded typescript worker, including a sample completion injection
camargo
force-pushed
the
fix/289/ts_worker_extension
branch
from
May 16, 2023 21:53
c9b7284
to
277d944
Compare
camargo
force-pushed
the
fix/289/ts_worker_extension
branch
from
May 16, 2023 21:57
277d944
to
c381214
Compare
camargo
force-pushed
the
fix/289/ts_worker_extension
branch
from
May 16, 2023 22:02
c381214
to
db2200d
Compare
camargo
reviewed
May 16, 2023
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.
🚀
camargo
approved these changes
May 16, 2023
camargo
force-pushed
the
fix/289/ts_worker_extension
branch
from
May 16, 2023 23:52
db2200d
to
8a9be31
Compare
JosephVolosin
pushed a commit
that referenced
this pull request
Aug 20, 2024
* Add esbuild for correctly bundling workers * Modify MonacoEditor.svelte to use custom TS worker * Includes a minimal overridded typescript worker, including a sample completion injection * Fix typing for custom methods * Built workers in `npm run build`, ensure worker config does not crash
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #289
PR 1/2, this commit contains a minimum viable custom TS worker. Next PR will include the demo from https://github.com/NASA-AMMOS/aerie-monaco-editor-customizations, of validating timestamps.
Summary
This PR creates and integrates a custom typescript worker for Monaco that does not require maintaining a fork of Monaco, does not require redoing any of the hard language server work, and also taking the blessed path from Monaco for doing this sort of thing. It will allow us to add new diagnostics and should allow us to differentiate different editor instances on the same page!
How it works
We get to create a subclass of the Monaco TypeScript worker in https://github.com/NASA-AMMOS/aerie-ui/blob/5c9130f7ab85cc3682a8715cb6a24803cf6ea2f0/src/utilities/customTS.worker.ts and conditionally override only the methods required for our work. Currently, I have a barebones subclass with just a few methods implemented, although you can just ctrl click on the types to see the other possible methods.
We still load the original Monaco TypeScript worker code straight out of node_modules as before, so we will automatically get any backend changes included in changes when we update Monaco. This method, of providing a subclass factory function, is the intended (if very very poorly documented) solution from the Monaco team for doing this.
Build System Changes
Doing this (specifically specifying a
customWorkerPath
for the typescript worker defaults, which loads our subclass factory) requires that the original worker be launched inclassic
mode, which is where we run into issues with Vite. It will not properly bundle worker files for this mode in development no matter what you do, so we require a new Vite plugin that hooks into file changes and rebuilds the worker on changes, then forces a reload.This currently works pretty well, with the obvious downside that it forces a full page reload and the less obvious issue that it will not automatically reload on changes of files imported by the workers, just on the worker files themselves. I don't believe we can work around the page reload issue easily due to the realities of worker lifetimes. I may be able to figure out how to get esbuild to tell me what other files need to be watched with some more effort, but right now I'm not sure that's worthwhile given that this stuff probably won't be changed all that frequently and we can just
cmd+r
if you need to.Pain Points
Currently, there's one big pain point - Worker Typing. Because of the issues with the Monaco build process, there is no way to get the relevant types for the TypeScript Worker class from the Monaco project's release artifacts, they must come from an intermediate build step and be manually copied. I have done that already, however it is possible that the reality of the underlying data may drift from the types at some point in the future. I believe the only real fix here is testing, as we'd need to get Monaco to make a rather substantial change in their exported interfaces.
Additionally, there's a bit of weirdness in return types - the Diagnostic type in
typescript
is not actually JSON serializable due to internal references to file objects, so despite the types working out, you need to ensure you match the type definition for Diagnostic in the modulemonaco/languages/typescript
which includes only thefileName
parameter forfile
. (this will make more sense when I create PR 2/2 with the time string format validation).