suren-atoyan / monaco-react

Monaco Editor for React - use the monaco-editor in any React application without needing to use webpack (or rollup/parcel/etc) configuration files / plugins
https://monaco-react.surenatoyan.com/
MIT License
3.73k stars 265 forks source link

🐛 exposed Monaco Editor and `onChange` do not reflect actual state of Monaco contents #402

Open heysweet opened 2 years ago

heysweet commented 2 years ago

Describe the bug

The underlying Monaco state in React is not accurately reflected in scenarios where users make a change, and then quickly un-make that change. This state does not ever resolve itself unless a user intercepts and makes an additional change.

It's possible this is a bug in Monaco itself, but hopefully upgrading the version of Monaco could help resolve this.

To Reproduce

Steps to reproduce the behavior:

  1. Put a console.log(contents) in the onChange= of Editor
  2. Type a character (like "T")
  3. Quickly hit backspace
  4. Notice that onChange only fires once.

You could also grab the editor object from onMount and run getValue() at this time -- note that the file contents will still have a "T" in them from your edited changes.

(If you keep alternating between "T" and "Backspace" you will notice that it will fire onChange for every other change, the addition of the "T", or if you type a T ... wait a few seconds and then start alternating "Backspace" and then "T" you will only get the backspace onChange events.)

Expected behavior Any change should be captured accurately in onChange. At a minimum, the exposed editor object should be able to get the accurate file contents in these "change/unchange" scenarios

heysweet commented 2 years ago

Ways to reproduce:

suren-atoyan commented 2 years ago

hey 👋 I couldn't reproduce it. could you please check this snippet and try to reproduce it here?

codesiddhant commented 2 years ago

i also noticed that bug. actually when ever we change the value of editor and again reset to initial value, it seems onChange unable to notice that

hey wave I couldn't reproduce it. could you please check this snippet and try to reproduce it here?

to reproduce it. just cut everything on editor and then paste again you'll notice that it will unable to notice that change

codesiddhant commented 2 years ago

hey wave I couldn't reproduce it. could you please check this snippet and try to reproduce it here?

to reproduce it. just cut everything on editor and then paste again you'll notice that it will unable to notice that change

image i think the bug is resolved by just commenting these line, please current me if i am doing any thing wrong here

src/editor.js -> line no. 136 to 138

suren-atoyan commented 2 years ago

@heysweet @codesiddhant could you please check the latest (v4.4.6) version?

heysweet commented 2 years ago

I can't give a definitive answer on if this fixes what I need fully, but after some cursory exploration, it does seem to eliminate this bug!

I ended up getting into a bad state with value= and infinite re-render but this could be on my side.

Unfortunately, I don't have the bandwidth to more fully explore this immediately, but will try to get back to this when I can. Thanks for the bugfix!

suren-atoyan commented 2 years ago

I ended up getting into a bad state with value= and infinite re-render but this could be on my side.

@heysweet this is really important, let me know once you have more information about this.

heysweet commented 2 years ago

Hi! I've managed to confirm the original bug is resolved, i.e., I can now trust onChange to be fired with updated contents. However, value= does not overwrite the contents monaco when I need it to reliably, so I still need a workaround to force content overwrites into monaco :(

heysweet commented 2 years ago

@suren-atoyan I'll outline my understanding and hopefully this can help!

We are avoiding use of the value= param altogether in favor of interacting with monaco itself because of this issue --

defaultValue= works as expected! value= has a lingering state problem that makes things difficult for us. We're writing a Web IDE that has git interactions, i.e. at a minimum example, the ability to revert. I've approached value= with 3 strategies, none of which works:

  1. Only update value= explicitly when we want to override what the user has in there. This works fairly well but has a big problem. Let's say that the last commit was just the string "A". (value="A"). The user replaces this with "B". (value="A"). When the user attempts to hit the revert button, we have no means of interfacing with the react component to tell it "update your value!" since the value we want to update it to is the already the value being passed in.

  2. Attempt to update value= all the time to avoid this earlier problem. Every keystroke, we call a debounce function which writes to our file cache, so ~50ms later we get the state the user had typed in. If we pass in this updated value into value= while the user is typing, we will continually overwrite what the user is typing, or even if we shorten this feedback loop, we had a number of bugs reported around the cursor jumping around while the user is typing because of this configuration

  3. Only set value= to a string for 1 render in order to force an overwrite and then switch value={undefined} for all other times. This was difficult to manage state and ultimately anti-react-pattern-y so ultimately we've decided to fully abandon using the value/defaultValue interface in favor of just interfacing with the monaco instance and its models directly.