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 onTextChanged in MonacoEditorReactComp #684

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

vrama628
Copy link
Contributor

Fixes #683

Updates MonacoEditorReactComp to correctly handle changes to the onTextChanged prop.

Note: since there isn't a test suite for the React component on main, this branch includes changes from both this PR and #681 . See here for a diff of just the changes relevant to this PR.

The first commit adds a test case that reproduces the issue, and the second commit fixes it. When I run the test after the fix, Vite seems to get stuck and infinitely hang; I wasn't able to figure out why. I'm open to suggestions on how to fix the test case to run properly in vitest, or I can just remove the test case altogether.

@kaisalmen
Copy link
Collaborator

Hi @vrama628 let's merge #682 first, then you can rebase this PR. I will try to understand why the test fails and see if there is a solution or workaround.

@vrama628
Copy link
Contributor Author

Thanks, I'll convert this to a draft until #682 merges and I rebase this

@vrama628 vrama628 marked this pull request as draft June 25, 2024 16:12
@kaisalmen
Copy link
Collaborator

kaisalmen commented Jun 25, 2024

@vrama628 I just squash merged #682

@vrama628 vrama628 marked this pull request as ready for review June 25, 2024 19:34
@vrama628
Copy link
Contributor Author

@kaisalmen thanks, I've rebased and marked as ready for review

@kaisalmen
Copy link
Collaborator

kaisalmen commented Jun 26, 2024

@vrama628 interesting, the test only fails on Linux, but not on Windows. @CGNonofr looking at the error message do you have a spontaneous idea (we are not on v6, yet btw)?
image

@CGNonofr
Copy link
Collaborator

@vrama628 interesting, the test only fails on Linux, but not on Windows. @CGNonofr looking at the error message do you have a spontaneous idea (we are not on v6, yet btw)? image

Those unable to load extension-file:///... errors are hard to debug because VSCode hides the actual file it's trying to load. There is a fetch that is failing somewhere, you need to find it

@kaisalmen
Copy link
Collaborator

@CGNonofr thank you. I guessed that I have to debug it, but there was a small chance you had an idea. Updating to the newest version did not change the behaviour, btw

@CGNonofr
Copy link
Collaborator

@CGNonofr thank you. I guessed that I have to debug it, but there was a small chance you had an idea. Updating to the newest version did not change the behaviour, btw

Is this a jest test or something? did you override the fetch behavior?

@kaisalmen
Copy link
Collaborator

kaisalmen commented Jun 26, 2024

Is this a jest test or something? did you override the fetch behavior?

It is vitest in headless browser mode using chrome via webdriver.io

@CGNonofr
Copy link
Collaborator

Is this a jest test or something? did you override the fetch behavior?

It is vitest in headless browser mode using chrome via webdriver.io

Ok, I'm not familiar at all with it, I won't be able to help any further, sorry

@vrama628
Copy link
Contributor Author

It looks like updateExtendedAppPrototyp from the monaco-editor-wrapper test suite is designed to deal with this exact issue in tests -- could that help here? I think I tried that but then ran into a different issue where vite would hang

@kaisalmen
Copy link
Collaborator

@vrama628 that is exactly the issue. I didn't see that yesterday. I will merge this now and post a fix on main.

Copy link
Collaborator

@kaisalmen kaisalmen left a comment

Choose a reason for hiding this comment

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

LGTM. Test will be fixed on main

@kaisalmen kaisalmen merged commit aa7c6b8 into TypeFox:main Jun 27, 2024
1 check failed
@kaisalmen
Copy link
Collaborator

Build pipeline is green again: https://github.com/TypeFox/monaco-languageclient/actions/runs/9702661785

@kaisalmen
Copy link
Collaborator

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.

MonacoEditorReactComp not updating onTextChanged callback
3 participants