JedWatson / react-codemirror

Codemirror Component for React.js
MIT License
1.55k stars 262 forks source link

onChange fires when value is changed programatically #5

Open insin opened 9 years ago

insin commented 9 years ago

onChange is called when the Codemirror component receives a new value when rendering.

Test case: https://gist.github.com/insin/9e507894f3858028c7e5 Live version: http://bl.ocks.org/insin/raw/9e507894f3858028c7e5/

Every call to onChange is alert()-ed, Clicking the "Change value" button changes the value in the component's state.

Just got bit by this using Redux, as updating some state which was being passed to Codemirror resulted in another dispatch firing via onChange, resulting in old state being retained. Worked around it myself by having onChange check if it received the same value as was being used for rendering, but is this behaviour something that's possible to control, or it is a consequence of how Codemirror itself is implemented?

atis-- commented 8 years ago

The example doesn't really work for me (some errors in the console). However, the change event from CodeMirror instance has a second argument which describes the change. If value comes from setValue() call, then change.origin == 'setValue', so I think that may be used to differentiate between the changes.

alexdmiller commented 8 years ago

I've been battling with this for a while, and it's messy. I see state stored in three separate places:

  1. The CodeMirror object;
  2. The react-codemirror React component (in the _currentCodemirrorValue property);
  3. And my own component's state.

Synchronizing these three pieces of state is a nightmare. The way the state is sent to and from each object is unpredictable. I sunk a few hours into trying to patch up the state synchronization and haven't figured it out yet. Instead, I think we should (a) admit that having three separate "sources of truth" is a bad code smell, and (b) restructure the flow of data in the react-codemirror component.

The CodeMirror library is going to maintain its own internal state; there's no easy way to get around that. Therefore, the other state, in react-codemirror and the client app, need to go. I think this implies that the react-codemirror component should not expose a value attribute, just a defaultValue.

Am I off base here?

alexdmiller commented 8 years ago

Here's a test case that demonstrates this failure-to-synchronize: https://jsfiddle.net/n1gz4hLn/1/

Watch out, this will crash your tab. When you try to type, you enter an infinite loop.

alexdmiller commented 8 years ago

I took a stab at fixing this by removing the state from the CodeMirror React component. It's not perfect, but removes a lot of the code smell.

35

BerkeleyTrue commented 8 years ago

The PR reference by this issue has been merged. Should this issue be closed?

scniro commented 7 years ago

Hey all. I too was struggling with some related issues and decided to roll another wrapper for this at react-codemirror2. No external deps, state free, and works well for my use case. Please give it a look if you are feeling adventurous. I'd like to actively maintain it moving forward. I'm also looking for suggestions to craft it in the best fashion moving forward, so feel free to open up an issue with any wants/tips/etc you may want. Thanks!