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: replace all useEffect and useCallback with one useEffect #862

Merged
merged 1 commit into from
Feb 15, 2025

Conversation

ahmed-s-fatahallah
Copy link
Contributor

Changes Made:

  • Replaced multiple useEffect and useCallback hooks with a single, cohesive useEffect.
  • Improved container reference management and initialization.
  • Streamlined error handling and disposal processes for better reliability.

Impact:

These adjustments are intended to reduce code complexity, making the lifecycle management of the Monaco editor component more straightforward and maintainable.

Please review these changes and provide any feedback. Thank you!

@ahmed-s-fatahallah
Copy link
Contributor Author

Thank you for your suggestion to group each step into separate functions and call them inside an async IIFE. As mentioned in my last comment, I believe that declaring these functions outside the main useEffect might necessitate wrapping them in useCallback, which I think we can avoid.

I appreciate your patience and help!

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.

We are almost there, we need to properly handle destruction. All my comments are basically related to this.
I tested your changes and the dispose is no longer working properly.

@ahmed-s-fatahallah
Copy link
Contributor Author

@kaisalmen First of all thanks for your comments.
However, wouldn't it be simpler to move the destroyMonaco invocation inside the main useEffect cleanup function, rather than creating a new useEffect with an empty dependencies array just for it?

Also, moving all methods outside the main useEffect will require wrapping them inside a useCallback which I think we can avoid by just including the methods inside the main useEffect.

Lastly, could you please share how you tested the destroyMonaco function? I want to ensure I test it before pushing, as all unit tests were passing with my last push.

@kaisalmen
Copy link
Collaborator

kaisalmen commented Feb 15, 2025

Also, moving all methods outside the main useEffect will require wrapping them inside a useCallback which I think we can avoid by just including the methods inside the main useEffect.

@ahmed-s-fatahallah it is this example. It has start and dispose buttons:
https://typefox.github.io/monaco-languageclient/ghp_react_statemachine.html
or locally:
https://github.com/TypeFox/monaco-languageclient/blob/main/packages/examples/react_statemachine.html

@kaisalmen
Copy link
Collaborator

@ahmed-s-fatahallah I have updated all comments and if you use the clean-up function for the useEffect like you suggested this PR is ready to be merged. 🎉

@ahmed-s-fatahallah
Copy link
Contributor Author

@ahmed-s-fatahallah I have updated all comments and if you use the clean-up function for the useEffect like you suggested this PR is ready to be merged. 🎉

@kaisalmen
I have pushed the clean-up function change

Thanks a lot for your patience and support, and thanks a lot for this amazing repo 🙏

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! Thank you @ahmed-s-fatahallah and sorry for the bumpy ride due to the rebases.

@kaisalmen kaisalmen merged commit 7488efc into TypeFox:main Feb 15, 2025
1 check passed
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