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

fix: ts worker customWorkerPath error by exporting the TypescriptWorker class #3152

Closed
wants to merge 21 commits into from

Conversation

okikio
Copy link

@okikio okikio commented Jun 17, 2022

This pr is meant to fix issue #3151 in which the TypescriptWorker class isn't exported, reducing the ability for devs to extend monaco in a module worker, as importScripts (which the customWorkerPath uses to extend the typescript worker) isn't available for module workers.

@ghost
Copy link

ghost commented Jun 17, 2022

CLA assistant check
All CLA requirements met.

@okikio okikio changed the title fix: export TypescriptWorker class fix: typescript worker errors with customWorkerPath in module workers by exporting the TypescriptWorker class Jun 17, 2022
@okikio okikio changed the title fix: typescript worker errors with customWorkerPath in module workers by exporting the TypescriptWorker class fix: ts worker customWorkerPath error by exporting the TypescriptWorker class Jun 17, 2022
@hediet hediet added the info-needed Issue requires more information from poster label Jul 21, 2022
@okikio
Copy link
Author

okikio commented Jul 21, 2022

@hediet The importScript syntax doesn't work in esmodule workers, by allowing the TypeScript class to be exported I can make the TypeScript class work in esmodule workers even if the Monaco team doesn't officially support esmodule workers.

The problem is that customWorkerPath doesn't work in esmodule workers and unfortunately my situation requires that I use module workers.

hediet
hediet previously approved these changes Aug 3, 2022
@hediet hediet removed the info-needed Issue requires more information from poster label Aug 3, 2022
alexdima
alexdima previously approved these changes Aug 3, 2022
@hediet
Copy link
Member

hediet commented Aug 3, 2022

@okikio Can you look into why CI fails?

@okikio
Copy link
Author

okikio commented Aug 3, 2022

@hediet For some reason the CI complains that esbuild hasn't been configured for .ttf files, but I never touch .ttf files in this pr

@hediet hediet enabled auto-merge August 3, 2022 15:24
@hediet
Copy link
Member

hediet commented Aug 3, 2022

The CI on main is green though, thus the failure must be caused by this PR.

auto-merge was automatically disabled August 4, 2022 05:18

Head branch was pushed to by a user without write access

@okikio okikio dismissed stale reviews from alexdima and hediet via 4d560b2 August 4, 2022 05:18
@okikio
Copy link
Author

okikio commented Aug 4, 2022

@hediet I've pushed a change to the package-esbuild.ts file that tells esbuild to treat the codicon.ttf file as an external file, from local testing it seems to fix the problem, but I'd need your opinion before confirming if the solution works.

@okikio okikio requested review from alexdima and hediet August 4, 2022 05:20
@hediet
Copy link
Member

hediet commented Aug 5, 2022

Do you know why this problem happens here, but not in main?

@okikio
Copy link
Author

okikio commented Aug 5, 2022

I have no idea why it's doing this

@YousefED
Copy link

YousefED commented Aug 9, 2022

I'm running into a similar issue. My codebase uses a custom TS Worker as well, e.g.:

import { TypeScriptWorker } from "monaco-editor/esm/vs/language/typescript/tsWorker";

export class CustomTypeScriptWorker extends TypeScriptWorker {
...

Which worked on 0.27.0, but fails after upgrading to the latest version

@camargo
Copy link

camargo commented Sep 23, 2022

Any update on this one?

@okikio
Copy link
Author

okikio commented Sep 23, 2022

@camargo Thanks for reminding me, I'll look into this again

@okikio
Copy link
Author

okikio commented Sep 23, 2022

After a bunch of trial and error, I've determined that the cause of this issue is due to the exports being made, but I can't determine why the extra exports are causing the smoketests to fail

@okikio
Copy link
Author

okikio commented Sep 23, 2022

@camargo I'll need some help here as I'm unable to debug why the playwright tests keep crashing when I export functions that aren't create

@camargo
Copy link

camargo commented Feb 1, 2023

Hi alexdima and okikio. We would love to use this feature if possible. Any ideas how to get this PR working? Happy to help get it through just need some guidance on where to start. Thanks!

@okikio
Copy link
Author

okikio commented Feb 1, 2023

@camargo The pr itself is working, it's the test that's broken and I can't figure out how to fix it

@camargo
Copy link

camargo commented Feb 1, 2023

@alexdima Any advice on how to fix the test?

@joswig
Copy link

joswig commented Apr 12, 2023

Getting this merged would be great for our project.

Is there any chance of getting this assigned to a monaco milestone?

@hediet
Copy link
Member

hediet commented Jul 7, 2023

Closing in favor of #4035

@hediet hediet closed this Jul 7, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants