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

refactor: enhance code performance, maintainability and extendibility #847

Closed
wants to merge 0 commits into from

Conversation

ahmed-s-fatahallah
Copy link

replace the too many useEffect and useCallbacks with one useEffect to instantiate and update the editor and another effect to dispose the models' Subscriptions when the component unmounts.

@kaisalmen
Copy link
Collaborator

Hi @ahmed-s-fatahallah thank for this PR. It made me realize the subscriptions for the onDidChangeContent should not be in the react component at all. I opened #849 to correct that.

Please fix the linting problems, then we can review the changes.

@ahmed-s-fatahallah
Copy link
Author

Hi @ahmed-s-fatahallah thank for this PR. It made me realize the subscriptions for the onDidChangeContent should not be in the react component at all. I opened #849 to correct that.

Please fix the linting problems, then we can review the changes.

You are welcome, I will fix the linting problems as soon as possible.

Looking forward for your review

@kaisalmen
Copy link
Collaborator

@ahmed-s-fatahallah please rebase your PR. #849 removes the need to deal with subscriptions on the react level, therefore you need to change less code. Sorry for the inconvenience. I will review the changes after the rebase.

@ahmed-s-fatahallah
Copy link
Author

@kaisalmen Hello Kai,
It looks like the onTextChanged prop on the MonacoEditorReactComp isn't receiving the textChanges parameter after the latest update. Should it capture the changes in the code editor? If yes, it seems to be broken; if no, can you clarify its purpose?

Thanks

@kaisalmen
Copy link
Collaborator

kaisalmen commented Feb 11, 2025

It looks like the onTextChanged prop on the MonacoEditorReactComp isn't receiving the textChanges parameter after the latest update.

Hi @ahmed-s-fatahallah we already had unit tests that verify onTestChanges and they are still green. Why do you come to that conclusion? I also tested it manually. There is one thing though: If you use the views service (appPlayground and python example (after merge of #842)) the callbacks don't get registered because "vscode" creates the editors depending which resources/uri is opened.

@ahmed-s-fatahallah
Copy link
Author

@kaisalmen Hi there,

I apologize, I was using the view service , but I've now switched to the editor service and encountered these console errors:

Screenshot (1)

It seems there's a missing service override that I should include. However, I'm having trouble locating this service. Could you please assist me in making the React: Python Language Client & Language Server (Web Socket) example work as an editor service?

Thank you!

@kaisalmen
Copy link
Collaborator

Hi @ahmed-s-fatahallah I can't tell from the screenshot (there are likely incompatible services loaded). But you can use this example:
https://github.com/TypeFox/monaco-languageclient/blob/main/packages/examples/src/langium/statemachine/main-react.tsx
It uses the EditorService (it's the default, btw) and already contains the callback. You don't have to change anything.

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.

2 participants