Change handlers should not fire because of prop change

[MartinHaeusler]

I finally got around to experiment with this component. Thanks for providing a react-redux friendly API via a controlled component. It works well so far, however I do have one issue.

Let's assume we have a react-codemirror2 instance in controlled mode (slightly adapted from your readme.md):

<CodeMirror
    value={reduxState.editor.value}
    options={options}
    onBeforeChange={(editor, data, value) => {dispatch(...state change...)}}
    onChange={(editor, metadata, value) => {}}
/>

It appears to work nicely when you use it regularly. However, if you try to use the "time travel" feature provided by redux, you notice something strange: the onBeforeChange(...) listener fires when the value prop changes. This creates new actions in the redux store during playback, which is clearly not intended.

Compare this to how <input type="text"/> works. Here, onChange(...) only fires when the actual user input changes, but changing the value prop programmatically will not fire the listener. This is one of the core concepts of flux/redux.

So, long story short, in order to make the behaviour consistent with all text areas and text inputs out there, please do not fire change listeners of any kind if the "change" was triggered programmatically via props. They also should not fire due to changes during the mounting of the codemirror component.

[scniro]

@MartinHaeusler hey thanks for opening this up! I played around with a simple <input/> this morning to wrap my head around what you are saying and yes I indeed see the inconsistency. While working through the Controlled variant I remember some difficulties surfacing because I need to carefully deal with the codemirror instance as well.

Hmm, perhaps on the controlled component, I can add a strict prop, which if true, should emulate the expected behavior consistent with an input. I don't wish to force this behavior across the component as a whole as it's working well for myself and others, but the flag could be a decent compromise.

Thoughts?

[MartinHaeusler]

First of all, thanks for the fast response! I had a quick look at your code and I can imagine that the controlled API was no cake-walk with respect to state management.

Making it consistent with <input> is definitly a good idea. I'm not sure if the strict boolean is required, because when using a controlled component, I don't think anybody expects callbacks to be activated due to a prop change (which was triggered by the programmer to begin with). I actually had quite a hard time figuring out what was going on in my app, I didn't expect this to happen :P So, yes, making it consistent with <input> is definitly a great idea, maybe this should be the only controlled mode there is. I do see your point with the boolean though... Maybe for backwards compatibility, and set strict to true by default?

[scniro]

@MartinHaeusler agreed! I was having a bit of difficulty getting onBeforeChange to fire programmatically, I guess because you are seeing this with your debug tools. Anyways, I threw up a binary to react-codemirror2-3.1.0-a. Can you try to install this locally to test the changes?

I took your suggestion and implemented the behavior by default and added a strict={false} if the user wants to respond to onChange by changing props. Please let me know if this looks okay and I'll publish up!

Thanks for your collaboration on this 😎

[MartinHaeusler]

Thanks for your efforts, that was really fast! I replaced my local node_modules/react-codemirror2 version with the contents of your archive. According to typescript, the strict property exists, so I guess I did that correctly? I only installed node modules directly from NPM so far...

My experiments so far show:

• Setting strict to true or false does not seem to make any difference regarding the listeners firing.
• The onBeforeChange is fired only when user input changes and ignores prop change (which is good), but this happens regardless of the strict setting (which is not your intention I think?)
• The other listeners, e.g. onSelection, onCursor etc. still fire on prop change, regardless of strict being true or false

My suggestion would be to set strict={true} by default, as new users will expect the same behaviour as <input>. Existing users just need a remark in the readme.md when they upgrade. And, perhaps don't call it strict (that's rather generic), perhaps fireListenersOnPropChange would be more clear.

[scniro]

I didn't attend to any other listeners e.g. onSelection and others. The only difference should be, if strict={true} - onChange will fire for a new prop. If strict is not specified, you should not see it fire at all.

Perhaps try to reinstall? You should be able to just copy that tgz file into your project and npm install react-codemirror2-3.1.0-a.tgz

I'm staring to get a bit lost with everything going on now... 😦

[MartinHaeusler]

Oh okay, if your update was referring only to onBeforeChange, then it works (except that setting strict to false doesn't seem to change anything). However, the code in my app still does not function correctly because the other listeners fire on prop change. Could you perhaps give onSelection etc. the same treatment as onBeforeChange?

[scniro]

No, it's for onChange and nothing needs to be specified it's the default behavior. The only option in the binary I pushed up is to set it to false. I'm a bit burned out over this today and might pick it back up tomorrow. Pull requests welcome!

[MartinHaeusler]

I spent a LOT of time trying to fix this myself now. I just thought that I let you know in case you are interested. I started out with your implementation and eventually started over from scratch, but boy, achieving a controlled code mirror is truly a can of worms. Feels like catching every ant in an ant colony one by one. My current status is that I can properly control:

• the editor value (text)
• cursor position
• selection

... in a controlled component, with several tricks (including event buffering and reordering...). However, adding in the scroll position as a controlled aspect turns out to be super complicated. There are several different coordinate systems at work (window-relative, page-relative and local). To make matters worse, codemirror has some events you cannot easily hook into. For example, let's say your document has 200 lines. You see lines 0-50 in your current viewport. Your cursor is at 50, and you press the "down" arrow key on the keyboard (going to line 51). By default, codemirror now scrolls to follow the cursor, moving line 51 into view. However, the scroll event is not fired in this case. Same thing is true if you press the "enter" key in the same case. The scroll bar changes, yet the scroll event isn't fired.

As awesome as codemirror is, the event handling is a nightmare come alive, and the information within the events themselves is lacking at best.

[scniro]

@MartinHaeusler yea it's a pain to say the least - hence why I set this down for a little bit 😄 I do still like the suggestion of using onChange in place of onBeforeChange and I might be able to make that happen soon.

For most of the events I am totally fine with codemirror doing the heavy lifting for things such as selection, scroll, etc. I think firing the callback and letting the user set one is enough. I'll keep you posted with onChange updates which will likely be the kick-off for a 4.x release.

[scniro]

Closing for now as the scope of this discussion has ballooned into a potential 4.0 roadmap with various points of discussion. I also don't find this to be an immediate issue for general usage.

[orlowang]

It's still happen in my case, I use v4.1.0

[scniro]

@owcc I've decided to keep the API as-is. It's just too difficult and not worth the aggravation to adhere to a pure onChange callback. There are quite a few complications that need to happen for codemirror to work of which both of hooks in a controlled execution flow are needed.