Lifecycle adjustments by kaisalmen · Pull Request #939 · TypeFox/monaco-languageclient

kaisalmen · 2025-07-03T12:29:17Z

Remove monaco-editor-wrapper. Create EditorApp in client
- Move languageclient management from wrapper to client
- move all tests from wrapper to client
- client: proper separation of sub-exports, rename tools to common
monaco-vscode-api independent config and init
- clean up and restructure src and test
- Update vitest and other dependencies
Update examples
- Unify json, eclipse.jdt and groovy example
react component: fix re-render and global init
- expand react tests
- LanguageClient handling with react component

Fixes #919

kaisalmen · 2025-07-03T12:37:36Z

@CGNonofr I know this is a beast. The wrapper is gone. Everything apart from the react component is back in monaco-languageclient with multiple sub-exports handling the logical structure. monaco-vscode-api handling is clearer now, I think.

I have spent some effort to re-write unit tests for the react component. The life cycle issues should be gone and the tests prove that (at least that's my current hope 🙂)

CGNonofr · 2025-07-03T12:42:37Z

beast is probably an understatement ^^

I don't think I will be able to add any value reviewing this :-\ except maybe typos?

kaisalmen · 2025-07-03T12:44:59Z

I don't think I will be able to add any value reviewing this :-\ except maybe typos?

What about playing around with /checking the examples and focusing on the usability of the API and the meaningfulness of the tests.

CGNonofr · 2025-07-03T12:46:30Z

I don't think I will be able to add any value reviewing this :-\ except maybe typos?

What about playing around with /checking the examples and focusing on the usability of the API and the meaningfulness of the tests.

Is it urgent? I probably won't have the time today, and tomorrow is off

kaisalmen · 2025-07-03T12:48:29Z

No urgency, I would like to have your feedback / opinion. I will release next versions. This anyway goes back to the dev branch and not yet to main.

kaisalmen · 2025-07-03T12:49:55Z

We can release maintenance builds from main. This is the future, but it won't be ready before August. Time restrictions / vacaction on my end as well.

kaisalmen · 2025-07-03T13:10:45Z

Next releases are available. The api changed substantially. moanco-editor-wrapper is gone, but oveall fuctionality is still there. Check the examples and tests before proceeding. This is for evaluation purpose and not intended for production:
https://www.npmjs.com/package/monaco-languageclient/v/10.0.0-next.0
https://www.npmjs.com/package/@typefox/monaco-editor-react/v/7.0.0-next.0

montymxb · 2025-08-11T07:45:36Z

I'm going to start digging into this one a bit today, but I'll definitely need some time to go over it. I think I can get back with a concrete review tomorrow sometime.

- Remove monacp-editor-wrapper. Create EditorApp in client - Move languageclient management from wrapper to client - move all tests from wrapper to client - client: proper separation of sub-exports, rename tools to common - monaco-vscode-api independent config and init - clean up and restructure src and test - Update vitest and other dependencies - Update examples - Unify json, eclipse.jdt and groovy example - react component: fix re-render and global init - expand react tests - LanguageClient handling with react component

kaisalmen · 2025-08-11T11:53:19Z

@montymxb the rebase was completed successfully. The tests in GHA actions are suddenly unstable. They work locally. I will see if I can understand where the problem comes from. That should not stop you, though.

…es to monaco-editor-wrapper where applicable - Updated test timeout and vite optimizeDeps

montymxb

Alright really big PR to review, so I would consider this an initial pass with some points & suggestions. Overall the refactor looks to be good in terms of separating responsibilities that were previously co-mingled, but there's a lot of complexity present with the state management logic.

We should definitely include a migration guide as well as new docs about how to setup an editor using the new approach outlined here. I would be on board to help with that, time permitting.

montymxb · 2025-08-12T12:20:34Z

Adding a CoPilot review as well to see if anything else comes up that I missed on the first pass.

Copilot

Pull Request Overview

This PR performs a major architectural refactoring to remove the monaco-editor-wrapper package and consolidate its functionality into the client package. The changes involve moving EditorApp functionality, language client management, and testing to the client, while updating examples and components to use the new structure.

Key changes:

Removes the entire monaco-editor-wrapper package and consolidates EditorApp functionality into the client
Moves language client management from wrapper to client with a new manager architecture
Updates all examples to use the unified configuration approach and removes duplicate implementations
Restructures React component to work directly with monaco-languageclient instead of the wrapper

Reviewed Changes

Copilot reviewed 119 out of 123 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
vitest.config.ts	Updated test timeout and reorganized test include patterns to reflect new package structure
vite.config.ts	Removed extensive monaco-vscode package includes and simplified to core dependencies
packages/wrapper/*	Complete removal of monaco-editor-wrapper package including all source, tests, and configuration
packages/wrapper-react/src/index.tsx	Major rewrite to use EditorApp, LanguageClientsManager, and MonacoVscodeApiWrapper directly
packages/examples/src/*/client/main.ts	Updated all example clients to use unified configuration approach
packages/examples/src/common/client/extendedClient.ts	New shared client implementation for common example patterns

kaisalmen · 2025-08-17T11:59:36Z

@montymxb I have completed the review comment implementation. Do you agree with the following? #939 (comment)
For everything else, please check the delta on the latest commit. It contains all changes regarding the review.

I still have no idea why the build pipeline aborts the test. Everything is smooth locally.

montymxb

Thanks for the changes, looks pretty good to me now. The CI build issue is quite strange however, would be good to find out exactly what's going on there still.

One small typo, can be ignored too.

kaisalmen · 2025-08-18T07:53:52Z

@CGNonofr do you have any idea why the build pipeline gets cancelled? it is always the same error. For example see here: https://github.com/TypeFox/monaco-languageclient/actions/runs/17034213574/job/48282985642

Honestly, I have no idea right now. Web searches did reveal the issue is not new and this sort of cancellation happened before in other projects, but I don't understand why it suddenly happens here. Test locally are stable on this branch. Some timing/worker issue are actually gone/better here than on main. 🤷‍♂️

CGNonofr · 2025-08-18T08:15:54Z

@CGNonofr do you have any idea why the build pipeline gets cancelled? it is always the same error. For example see here: https://github.com/TypeFox/monaco-languageclient/actions/runs/17034213574/job/48282985642

Since it happens after exactly 1 minute, I would say a timeout somewhere?

kaisalmen · 2025-08-18T08:33:57Z

Since it happens after exactly 1 minute, I would say a timeout somewhere?

That is a coincidence. In other runs it gets cancelled after 1m5s oder longer. The problem only occurs on this branch not on main. It must somehow be related to the test changes, but don't understand yet how.

kaisalmen · 2025-08-18T15:07:29Z

@CGNonofr and @montymxb maybe the build cancellation issue is a resource issue after all. Locally, I see that headless_shell (from Chromium / playwright) consumes massive amounts of memory (8-10 GBs) intermediately during tests.

kaisalmen · 2025-08-19T15:21:57Z

@CGNonofr and @montymxb I have fixed the problem. I did not dispose the editor properly in many of the EditorApp related tests. This provoked a memory spike in Playwright/Chromium. Test execution times were improved by that as well.

…aywright/Chromium)

- Remove monaco-editor-wrapper. Create EditorApp in client - Move languageclient management from wrapper to client - move all tests from wrapper to client - client: proper separation of sub-exports, rename tools to common - monaco-vscode-api independent config and init - clean up and restructure src and test - Update vitest and other dependencies - Update examples - Unify json, eclipse.jdt and groovy example - react component: fix re-render and global init - expand react tests - LanguageClient handling with react component - Updated versions to next.1 and updated dependencies. Removed references to monaco-editor-wrapper where applicable - Updated test timeout and vite optimizeDeps - Implemented review comments - GHA: Properly dispose editor app in tests (saves lots of memory in Playwright/Chromium)

kaisalmen force-pushed the ls-lifecycle branch from 15929af to b6b20f1 Compare July 3, 2025 12:33

kaisalmen marked this pull request as ready for review July 3, 2025 12:37

kaisalmen requested a review from CGNonofr as a code owner July 3, 2025 12:37

kaisalmen changed the title ~~WIP: Lifecycle adjustments~~ Lifecycle adjustments Jul 3, 2025

kaisalmen requested a review from montymxb July 3, 2025 12:38

kaisalmen mentioned this pull request Jul 8, 2025

Improve life-cycle / separate language client from wrapper and react component #919

Closed

kaisalmen force-pushed the dev branch from 84f830c to 698d22b Compare August 11, 2025 10:32

kaisalmen force-pushed the ls-lifecycle branch from b6b20f1 to ae18e6b Compare August 11, 2025 11:37

kaisalmen force-pushed the ls-lifecycle branch from ae18e6b to 90c7dcc Compare August 11, 2025 12:54

Updated versions to next.1 and updated dependencies. Removed referenc…

1898cb1

…es to monaco-editor-wrapper where applicable - Updated test timeout and vite optimizeDeps

kaisalmen force-pushed the ls-lifecycle branch from 90c7dcc to 1898cb1 Compare August 11, 2025 13:05

montymxb reviewed Aug 12, 2025

View reviewed changes

montymxb requested a review from Copilot August 12, 2025 12:20

Copilot AI reviewed Aug 12, 2025

View reviewed changes

Comment thread packages/examples/src/node.ts Outdated

Comment thread packages/wrapper-react/test/index.test.tsx Outdated

Comment thread packages/wrapper-react/test/index.test.tsx Outdated

Comment thread packages/wrapper-react/test/index.test.tsx Outdated

kaisalmen force-pushed the ls-lifecycle branch 2 times, most recently from 7680a7e to 124b3d2 Compare August 17, 2025 11:56

montymxb approved these changes Aug 18, 2025

View reviewed changes

Comment thread packages/client/src/editorApp/editorApp.ts Outdated

Implemented review comments

77fb166

kaisalmen force-pushed the ls-lifecycle branch from 124b3d2 to 77fb166 Compare August 18, 2025 07:38

kaisalmen force-pushed the ls-lifecycle branch 7 times, most recently from cb5d419 to b233e29 Compare August 19, 2025 15:00

GHA: Properly dispose editor app in tests (saves lots of memory in Pl…

b6f9300

…aywright/Chromium)

kaisalmen force-pushed the ls-lifecycle branch from b233e29 to b6f9300 Compare August 19, 2025 15:24

kaisalmen merged commit 200aa05 into dev Aug 19, 2025
1 check passed

kaisalmen deleted the ls-lifecycle branch August 19, 2025 18:16

Uh oh!

Uh oh!

Conversation

kaisalmen commented Jul 3, 2025 • edited by montymxb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaisalmen commented Jul 3, 2025

Uh oh!

CGNonofr commented Jul 3, 2025

Uh oh!

kaisalmen commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CGNonofr commented Jul 3, 2025

Uh oh!

kaisalmen commented Jul 3, 2025

Uh oh!

kaisalmen commented Jul 3, 2025

Uh oh!

kaisalmen commented Jul 3, 2025

Uh oh!

montymxb commented Aug 11, 2025

Uh oh!

kaisalmen commented Aug 11, 2025

Uh oh!

montymxb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

montymxb commented Aug 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaisalmen commented Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

montymxb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaisalmen commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CGNonofr commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaisalmen commented Aug 18, 2025

Uh oh!

kaisalmen commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaisalmen commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

kaisalmen commented Jul 3, 2025 •

edited by montymxb

Loading

kaisalmen commented Jul 3, 2025 •

edited

Loading

montymxb left a comment •

edited

Loading

kaisalmen commented Aug 17, 2025 •

edited

Loading

montymxb left a comment •

edited

Loading

kaisalmen commented Aug 18, 2025 •

edited

Loading

CGNonofr commented Aug 18, 2025 •

edited

Loading

kaisalmen commented Aug 18, 2025 •

edited

Loading

kaisalmen commented Aug 19, 2025 •

edited

Loading