Rerenders cause editor element to be recreated for MonacoEditorReactComp

[KamWithK]

I created a reproducible example with a purple button where every time you click it will update the text and cause another div to be created:

https://github.com/KamWithK/monaco-languageclient-demo

[kaisalmen]

Hi @KamWithK thank you for reporting. I didn't see the solution at first. You have to move the config outside the app, otherwise React sees the need to re-render everything all the time. Use the modifiedTextValue for the stateChange. This was introduced with v7.

const languageClientConfig = createLanguageClientConfig();
const vscodeApiConfig = createVscodeApiConfig();
const editorAppConfig = createEditorAppConfig({
    uri: "/workspace/example.statemachine",
    text,
});

const App = () => {
    const [testState, setTestState] = useState<string>(text);
    return (
        <>
            <button
                style={{background: "purple"}}
                onClick={() => setTestState(testState + "test")}
            />
            <div>
            <div>{testState}</div>
            <MonacoEditorReactComp
                style={{height: "50vh", width: "100vh"}}
                vscodeApiConfig={vscodeApiConfig}
                editorAppConfig={editorAppConfig}
                languageClientConfig={languageClientConfig}
                modifiedTextValue={testState}
            />
            </div>
        </>
    );
};

[KamWithK]

I am aware that you can structure your code that way and it avoids the problem, but don't think that would work for us

Our actual code for example passes in extra editor options for theme which can change at runtime

It would be good if you just had a editor and language client started ref, so you could fix the bug directly?

[kaisalmen]

@KamWithK I suggest to use the onVscodeApiInitDone, onEditorStartDone, onLanguageClientsStartDone callbacks to obtain the MonacoVscodeApiWrapper, the EditorApp or the LanguageClientsManager. Then you can apply your changes directly e.g. apply a new theme in monaco-editor or alter the vscode user config directly, for example.
I realized that it is not clear that the three configuration objects should only be used for initial configuration of the application. When you don't use the react component this is clear. It was a design choice for the react component we took to ensure the initialization and start-up order of all software pieces it wraps. We must ensure that a different usage does not break the behaviour as you demonstrated.

[KamWithK]

@KamWithK I suggest to use the onVscodeApiInitDone, onEditorStartDone, onLanguageClientsStartDone callbacks to obtain the MonacoVscodeApiWrapper, the EditorApp or the LanguageClientsManager. Then you can apply your changes directly e.g. apply a new theme in monaco-editor or alter the vscode user config directly, for example. I realized that it is not clear that the three configuration objects should only be used for initial configuration of the application. When you don't use the react component this is clear. It was a design choice for the react component we took to ensure the initialization and start-up order of all software pieces it wraps. We must ensure that a different usage does not break the behaviour as you demonstrated.

I can't seem to find any other way to set the theme outside of the initial configuration, would you be able to point me towards it or provide a simple example?

[kaisalmen]

Hi @KamWithK you can acquire the EditorApp like this:

onEditorStartDone={(editorApp?: EditorApp) => {
    editorAppRef.current = editorApp;
}}

where editorAppRef is defined as such:

const editorAppRef = useRef<EditorApp>(undefined);

You can update any monaco-editor properties then directly from your app, like switching a theme (where for example the actual theme value could be bound to a state of you app):

editorAppRef.current?.getEditor()?.updateOptions({ theme: 'vs-dark'});

Does that help?

[KamWithK]

Hi @KamWithK you can acquire the EditorApp like this:

onEditorStartDone={(editorApp?: EditorApp) => {
    editorAppRef.current = editorApp;
}}

where editorAppRef is defined as such:

const editorAppRef = useRef<EditorApp>(undefined);

You can update any monaco-editor properties then directly from your app, like switching a theme (where for example the actual theme value could be bound to a state of you app):

editorAppRef.current?.getEditor()?.updateOptions({ theme: 'vs-dark'});

Does that help?

So I tried that but it doesn't seem to do anything for me

Also can still reproduce re-renders when moving config outside of React, just by hitting save in VScode many times (each time causes it to create a new visible element)

Would it be too much to ask you to fork my example repo and quickly demonstrate the intended way to do this with your library? Thank you for your help and assistance :)

[kaisalmen]

Also can still reproduce re-renders when moving config outside of React, just by hitting save in VScode many times (each time causes it to create a new visible element)

Is this reproducible with your demo above?

Would it be too much to ask you to fork my example repo and quickly demonstrate the intended way to do this with your library? Thank you for your help and assistance :)

Not immediately, but I will try this week. v7 already reduced the number of re-rendering and StrictMode issues we had, but there seem to be new ones and I would like to remove those issue. Also, if I have good re-production cases, I can distill unit test(s) from those.

[martin-fleck-at]

I think I might also be running into the same issue with strict mode. Everything works as expected without it but with strict mode, editors are added more than once and also other things do not work properly, e.g., I cannot use arrow keyboard navigation in the text or trigger auto-completion using Ctrl+Space.

I tried simply disposing the old editorApp when onEditorStartDone is triggered but it did not resolve the issue for me. It avoided the multiple rendering problem but the other issues were still present.

Btw, I'm really a big fan of the new version 10 and having everything more or less in one place again! Makes things a lot easier to maintain and the new separation of concerns really makes for a clean initialization, so thank you!

[kaisalmen]

I think I might also be running into the same issue with strict mode. Everything works as expected without it but with strict mode, editors are added more than once and also other things do not work properly, e.g., I cannot use arrow keyboard navigation in the text or trigger auto-completion using Ctrl+Space.

Hi @martin-fleck-at I currently see only completions in one of the two created editors in strict mode. That is either a bug a monaco-editor limitation we can't fix. I need to analyses this. That two editors are shown in StrictMode in general is correct, though.

There is an issue with language clients not being disposed when configured to do so with enforceDispose. This needs investigation.

Generally, I need to fix that config adjustment (triggered by react) lead to re-renders. Is anyone of you able to supply a unit test for this scenario that fails now, but should work?

Btw, I'm really a big fan of the new version 10 and having everything more or less in one place again! Makes things a lot easier to maintain and the new separation of concerns really makes for a clean initialization, so thank you!

Thank you. It's good to hear.

[martin-fleck-at]

Hi @martin-fleck-at I currently see only completions in one of the two created editors in strict mode. That is either a bug a monaco-editor limitation we can't fix. I need to analyses this. That two editors are shown in StrictMode in general is correct, though.

There is an issue with language clients not being disposed when configured to do so with enforceDispose. This needs investigation.

Hi @kaisalmen, thank you for the quick follow-up! Could you elaborate a bit on the fact that two editors shown are correct? I do understand that strict mode makes every hook execute twice but even if both editors were usable, I'd only ever expect a single editor instance per component.

If this issue is with enforceDispose then I actually may be facing a different problem. My use case is having a React application with multiple React editors but only a single language client, so I don't want to dispose it when any of the editors is disposed. Currently, we take care of initializing Monaco properly, registering our languages and themes and then setting up a single language client that connects to our language server. Based on that overall infrastructure, we render multiple editors to show different, editable, language-specific content. This all works when I'm not using strict mode (even though I always have to provide a vscodeApiConfig that will mainly be ignored if vscodeApiInitialising is already true except for logging). From what you are saying now, it sounds to me like you are suggesting not to use MonacoEditorReactComp in this case?

Edit: I just tried to create a unit test for it but I cannot reproduce it with that. Not sure whether it is my test setup or not. Just on the examples it is much easier to reproduce :(

[kaisalmen]

I do understand that strict mode makes every hook execute twice but even if both editors were usable, I'd only ever expect a single editor instance per component.

Thank you, that is correct and I think I just found the problem. I have to protect the internal logic to be run twice (editor creation in case of StrictMode). I thought I did that properly, but it seems, I didn't. The same applies to language client. I will work on a fix this week.

From what you are saying now, it sounds to me like you are suggesting not to use MonacoEditorReactComp in this case?

enforceDispose must be explicitly set. Per default the language client is not disposed. enforing disposal is just a possibility. It seems I oversaw more life-cycle / re-rendering bugs that effects the language client re-creation / disposal. ⬆️

Edit: I just tried to create a unit test for it but I cannot reproduce it with that. Not sure whether it is my test setup or not. Just on the examples it is much easier to reproduce :(

Thank you. Writing the react tests is not straightforward. Btw, it is possible to use non-headless mode and configure time-outs. Then you can observe what the browser does during the test. This can be really helpful to understand what is going on.

[KamWithK]

Also can still reproduce re-renders when moving config outside of React, just by hitting save in VScode many times (each time causes it to create a new visible element)

Is this reproducible with your demo above?

Take a look at my latest commit for a reproducable demonstration of my issues. The demo is in extended mode unfortunately, but I can still demonstrate the same problems nonetheless. I have changed the buttons action to try and toggle the minimap like you suggested for theme without any luck, and saving in editor will still produce a re-render.

Not immediately, but I will try this week. v7 already reduced the number of re-rendering and StrictMode issues we had, but there seem to be new ones and I would like to remove those issue. Also, if I have good re-production cases, I can distill unit test(s) from those.

That would be absolutely amazing! I hope my example can aid you in checking everything works fine.

[kaisalmen]

@KamWithK and @martin-fleck-at I started to work on this issue (see commit above). Editor is only rendered once in StrictMode now and also only one language client is created. Unit tests are broken and I need still need to fix/check the property based re-rendering problems. Getting there...

[martin-fleck-at]

@kaisalmen Thank you very much, that is great news! If you need me to test anything, please let me know!