Skip to content
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 6 commits into from
May 17, 2023
Merged

Implement Custom TS Worker #625

merged 6 commits into from
May 17, 2023

Conversation

Cobular
Copy link
Contributor

@Cobular Cobular commented May 8, 2023

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 in classic 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 module monaco/languages/typescript which includes only the fileName parameter for file. (this will make more sense when I create PR 2/2 with the time string format validation).

@Cobular Cobular requested a review from a team as a code owner May 8, 2023 19:29
@Cobular Cobular temporarily deployed to test-workflow May 8, 2023 19:29 — with GitHub Actions Inactive
@camargo camargo added the build Changes that affect the build system or external dependencies label May 8, 2023
vite.config.js Outdated Show resolved Hide resolved
vite.config.js Outdated Show resolved Hide resolved
vite.config.js Outdated Show resolved Hide resolved
@Cobular Cobular temporarily deployed to test-workflow May 15, 2023 17:32 — with GitHub Actions Inactive
@Cobular Cobular temporarily deployed to test-workflow May 15, 2023 22:35 — with GitHub Actions Inactive
@camargo camargo force-pushed the fix/289/ts_worker_extension branch from 339a8fd to c9b7284 Compare May 16, 2023 21:49
@camargo camargo force-pushed the fix/289/ts_worker_extension branch from c381214 to db2200d Compare May 16, 2023 22:02
@camargo camargo temporarily deployed to test-workflow May 16, 2023 22:02 — with GitHub Actions Inactive
Copy link
Member

@camargo camargo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@camargo camargo force-pushed the fix/289/ts_worker_extension branch from db2200d to 8a9be31 Compare May 16, 2023 23:52
@camargo camargo temporarily deployed to test-workflow May 16, 2023 23:52 — with GitHub Actions Inactive
@camargo camargo merged commit edd7bc1 into develop May 17, 2023
@camargo camargo deleted the fix/289/ts_worker_extension branch May 17, 2023 00:11
@Cobular Cobular mentioned this pull request Jun 5, 2023
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
Labels
build Changes that affect the build system or external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TypeScript worker extension to Monaco Editor for custom diagnostics
2 participants