Skip to content

Update complete uri and model handling in EditorApp #904

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kaisalmen
Copy link
Collaborator

@kaisalmen kaisalmen commented Apr 3, 2025

Changes:

  • EditorApp now always created models and disposes them properly when they are no longer needed. The logical structure of the code is better.
  • Fix onTextChanged handling in react component. This fixes Unexpected Behavior with State Changes in @typefox/monaco-editor-react 6.6.0 #898 => this required for changes / fixes in the Wrapper / EditorApp
  • Enforce proper uri usage in tests. There were many URIs that weren't unique
  • General api clean-up
  • Allow model dispose in EditorApp to be asynchronous in tests. This problem does not occur in the browser at all. vitest lets tests fail due to a " Unhandled Rejection / Canceled: Canceled" error that you cannot intercept when editor models are disposed. The only way to make the tests stable is to delay the model disposal if a timeout value is larger than 0.

The react component may need further polishing depending also on other observations from people I did not yet analyse, but we should go from here...

Copy link
Collaborator

@CGNonofr CGNonofr left a comment

Choose a reason for hiding this comment

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

Pretty huge commit 🤔

@kaisalmen
Copy link
Collaborator Author

Pretty huge commit 🤔

Yes, but model/uri handling was still fishy and that made this change bigger than initially expected. vitest gave me some headache as well.

…d handling in react component.

- Enforce proper uri usage in tests
- Allow model dispose in EditorApp to be asynchronous in tests
@kaisalmen
Copy link
Collaborator Author

@CGNonofr any thoughts? I have released next versions based on the current status, btw.

@CGNonofr
Copy link
Collaborator

CGNonofr commented Apr 4, 2025

@CGNonofr any thoughts? I have released next versions based on the current status, btw.

I'm sorry, I don't have as much time I'd like to be able to fully understand the changes you made. I'm sure it's for the best!

@kaisalmen
Copy link
Collaborator Author

@CGNonofr take your time. People can provide feedback based on the next packages. I also want to do some react component testing myself.

@zoran995
Copy link
Contributor

zoran995 commented Apr 4, 2025

Hi @kaisalmen I just tried upgrading to 6.7.0-next.0 and it still throws error in strict mode

Uncaught (in promise) Error: file 'extension-file://typefox.statemachine-example/extension/react-statemachine-configuration.json/' already exists

and I also get Extension '.liquid-example` is already registered, will try to figure out the reproduction steps, but posting as you might have some idea

Edit: Did you notice that in strict mode, you get component rendered twice (it doesn't happen in my instance but when I run the examples it does, also I tried to create an example vite project and it also rendered twice)?

@zoran995
Copy link
Contributor

zoran995 commented Apr 4, 2025

@kaisalmen I still believe there is no straightforward way to do this without implementing a task queue. Everything is asynchronous, and although we expect the component to initialize first and then dispose, we can't guarantee this sequence with asynchronous initialization and disposal.
Since useEffect doesn't support asynchronous functions, we need to make everything appear synchronous, which creates a problem in strict mode as React receives the useEffect callback before the initialization is completed. After receiving the callback, React expects that all work is completed and calls it, but in our case, it causes issues as the component doesn't know it should be destroyed and continues with initialization and starting.
With the task queue, we can ensure that the sequence of initialization and disposal is completed one after another, rather than in parallel. Basically you should be able to use the entire implementation from #872. I just tested it and couldn't reproduce the initialization issue; it seems to be fixed with something else.

@kaisalmen
Copy link
Collaborator Author

Hi @zoran995 I cannot reproduce the behaviour with a unit test, yet. I have locally defined another one that uses strict mode. I need to see if I make web worker language server run in a test (it did not work in the past if I remember correctly).

I am hesitant to introduce something complex to overcome a "dev" feature. I am not implying here that what it shows is wrong, but I am favoring a simpler solution if possible. There must have been something else been broken during the "simplification" in February (before 6.4.0 release) and I have an idea: You weren't able to restart during init and start combined. Now it is possible to do that I guess that introduced issues. I will come back...

@kaisalmen
Copy link
Collaborator Author

@CGNonofr and @zoran995 new preview builds (-next.1) are available.

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.

Unexpected Behavior with State Changes in @typefox/monaco-editor-react 6.6.0
3 participants