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

Update to monaco-vscode-api 9.0.x #749

Merged
merged 9 commits into from
Sep 26, 2024
Merged

Update to monaco-vscode-api 9.0.x #749

merged 9 commits into from
Sep 26, 2024

Conversation

kaisalmen
Copy link
Collaborator

@CGNonofr I have a weird observation. When I use monach TextEditorWorker is loaded and working and there is no error in the console.
When I use textmate, first I now need to specify the TextMateWorker otherwise there are errors printed (first screenshot). That wasn't required before. Once I do specify it, I see the warning in the second screenshot. The worker iscreated in the factory, but then something goes wrong. It can be reproduced with the langium example. It can be loaded with both configurations. I don't see this beahviour in the monaco-vscode-api demo locally. Do you have any idea?

No TextMateWorker provided:
image

TextMateWorker provided:
image

- Updated dependencies
- Streamlined LanguageClientWrapper#disposeLanguageClient due to a failing test after vitest update
@kaisalmen kaisalmen changed the title Update to monaco-vscode-api 9.0,x Update to monaco-vscode-api 9.0.x Sep 23, 2024
@CGNonofr
Copy link
Collaborator

CGNonofr commented Sep 23, 2024

When I use textmate, first I now need to specify the TextMateWorker otherwise there are errors printed (first screenshot). That wasn't required before.

Indeed, I've missed that change because it wasn't announced: there is a editor.experimental.asyncTokenization settings which allows to switch between local and worker syntax highlighting. That settings was false by default and was switched on recently

Once I do specify it, I see the warning in the second screenshot. The worker iscreated in the factory, but then something goes wrong. It can be reproduced with the langium example. It can be loaded with both configurations. I don't see this beahviour in the monaco-vscode-api demo locally. Do you have any idea?

Are you aware of the breaking changes of the v9.0.0 ? the error you get seems to be

throw new Error(`Unimplemented worker ${label} (${moduleId})`);

@kaisalmen
Copy link
Collaborator Author

Are you aware of the breaking changes of the v9.0.0 ? the error you get seems to be

Yes, the names are adjusted. That error above is not thrown. There is something async going on. When I halt in the debugger in the factory, it does not occur.

Indeed, I've missed that change because it wasn't announced: there is a editor.experimental.asyncTokenization

The settings are applied too late. That is different in your demo. I have to adjust.

@kaisalmen
Copy link
Collaborator Author

@CGNonofr update: Setting editor.experimental.asyncTokenization=false now works with the latest commit. Unfortunately, the unit tests now break, due to the init order changes. This is a vitest browser mode issue I need to circumvent.

Regarding the worker loading issue, I still need to investigate the root cause why loading the TextMateWorker fails.

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Sep 24, 2024

@CGNonofr I don't understand what actually the problem is. I have the suspicion the worker itself contains illegal code.

When w.addEventListener('error', onErrorCallback); is on invoked on the constructed TextMateWorker it breaks (see below, I updated the screenshot). You can reproduce this with any example when flipping the asyncTokenization flag.

Btw, I have reworked the logger and now rely on what monaco-vscode-api/vscode supplies. Also, the WorkerFactory is cleaned-up and simplified (dropping old, no longer needed config). 👍
image

@kaisalmen
Copy link
Collaborator Author

@CGNonofr let's defer the resolution of this problem (I could create a new issue) and merge this PR (after review, of course). Especially "unplanned" enhancements to logging, worker factory and start order make this PR very useful apart from the mva version upgrade.

packages/wrapper/src/editorAppClassic.ts Outdated Show resolved Hide resolved
packages/client/src/tools/index.ts Outdated Show resolved Hide resolved
packages/client/src/tools/logging.ts Outdated Show resolved Hide resolved
packages/client/src/vscode/services.ts Show resolved Hide resolved
@kaisalmen
Copy link
Collaborator Author

@CGNonofr this is ready. Any ideas regarding my findings: #749 (comment)

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented Sep 25, 2024

You can reproduce this by simply doing it manually. Something must happen in the worker and then the error is thrown, but the error content does not help me at least to understand why it dies:

const worker = new Worker(new URL('@codingame/monaco-vscode-textmate-service-override/worker', import.meta.url), { type: 'module' });
worker.addEventListener('error', (e) => {
    console.error(e);
});

@kaisalmen
Copy link
Collaborator Author

I tried to debug textMateTokenizationWorker.worker, but it does not halt on any breakpoints I tried. I see it created in the browser and when I halt in the callback it is still visible, but after basically already terminated. When you don't have that breakpoint it is gone immediately after it was instantiated. I have no experience in the inner workings of the SimpleWorkerServer / SimpleWorkerProtocol

packages/wrapper/src/vscode/services.ts Outdated Show resolved Hide resolved
packages/wrapper/src/vscode/services.ts Outdated Show resolved Hide resolved
@CGNonofr
Copy link
Collaborator

I tried to debug textMateTokenizationWorker.worker, but it does not halt on any breakpoints I tried. I see it created in the browser and when I halt in the callback it is still visible, but after basically already terminated. When you don't have that breakpoint it is gone immediately after it was instantiated. I have no experience in the inner workings of the SimpleWorkerServer / SimpleWorkerProtocol

Do you want me to have a look? Please tell me how to reproduce the issue like I am 5yo (:

@kaisalmen
Copy link
Collaborator Author

Please tell me how to reproduce the issue like I am 5yo (:

Sorry, it is too late for this 😆

Do you want me to have a look?

Yes, please have a look and reproduce it first and tell me I am not insane. It works in your demo. Something odd is going on here. You can use the Langium-DSL example and flip here or just the code snippet from #749 (comment) That is sufficient to reproduce.

@kaisalmen
Copy link
Collaborator Author

It dies right after this:
image

@kaisalmen
Copy link
Collaborator Author

@CGNonofr let's merge this. If you have any further idea regarding the issue above let's just write it down here or continue in a new issue. Both is fine with me.

One difference between your demo and the usage here is that you reference local packages/ locally build code and here the npm packages are used. That should no make a difference, but it is the only obvious difference.

@CGNonofr
Copy link
Collaborator

I'm not sure to understand, you want to merge something that's not fully working?

@kaisalmen
Copy link
Collaborator Author

When the Textmate Worker is disabled, then it is working fine. I could force that temporarily. WDYT?
I want those other changes in main, so I can continue with other work and release a next version.

We should fix/understand/find a solution for the issue before performing the next major version release.

@CGNonofr CGNonofr self-requested a review September 26, 2024 08:22
@kaisalmen kaisalmen merged commit 4896120 into main Sep 26, 2024
2 checks passed
@kaisalmen kaisalmen deleted the mva-next branch September 26, 2024 08:24
@CGNonofr
Copy link
Collaborator

@kaisalmen the solution is to add vscode-textmate and vscode-oniguruma in the vite config in optimizeDeps.include

The issue is that those libraries are still published in commonjs andthe worker is configured as type: module I think

@CGNonofr
Copy link
Collaborator

microsoft/vscode-textmate#240 🤷

@CGNonofr
Copy link
Collaborator

btw, I didn't find any way to debug workers in any browser, that's really a pain in the *ss. You just received an "Error" without any explanation nor details, while the error is that the worker code is not valid

@kaisalmen
Copy link
Collaborator Author

@CGNonofr awesome. It works. Thank you very much! How long did it take you to find out? Can I buy you a beer or something equivalent? 😆
image

btw, I didn't find any way to debug workers in any browse

Once they are loaded (that is sometimes like here the nasty point) you can add debug points and that works with Chrome. If you use a VSCode launch config that works (also in this project, btw).

@CGNonofr
Copy link
Collaborator

CGNonofr commented Sep 27, 2024

@CGNonofr awesome. It works. Thank you very much! How long did it take you to find out? Can I buy you a beer or something equivalent? 😆

No worries! :) It didn't really took me that long because I already had similar problem in the monaco-vscode-api demo some time ago

@cdietrich
Copy link
Contributor

@kaisalmen do we have a release eta for this one

@kaisalmen
Copy link
Collaborator Author

@cdietrich there are a pre-releases available (latest main commit):
https://www.npmjs.com/package/monaco-languageclient/v/9.0.0-next.1
https://www.npmjs.com/package/monaco-editor-wrapper/v/6.0.0-next.1
https://www.npmjs.com/package/@typefox/monaco-editor-react/v/6.0.0-next.1

No fixed ETA, yet

@cdietrich
Copy link
Contributor

cdietrich commented Oct 1, 2024

nice. does this one ring a bell
grafik
guess i have to use workerOverrides

but a naive

workerOverrides: {
            workerLoaders: {
              editorWorkerService: () => {
                return new Worker(new URL("../libs/monaco-editor-workers/editorWorker-es.js", import.meta.url), {
                  type: "module",
                })
              },
            },
          },

does not seem to work

@cdietrich
Copy link
Contributor

ahhh got it.
its now ignoreMapping: true, + TextEditorWorker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants