scniro / react-codemirror2

Codemirror integrated components for React
MIT License
1.65k stars 192 forks source link

on("changes") fails to fire under certain circumstances #139

Open justinpombrio opened 5 years ago

justinpombrio commented 5 years ago

This may be a stack management issue. It might also be a CodeMirror bug, though I haven't figured out how to reproduce it outside of react-codemirror2.

Background

In the project I work on, for every group of changes, we:

  1. Speculatively commit those changes to a temporary CodeMirror instance, then
  2. Wait for on("changes") to fire
  3. If the changes look valid, replay them on the real CodeMirror instance.

The Issue

We recently ran into a situation in which on("changes") fails to fire after a call to .operation(). Here's the minimal (though unfortunately rather long) code to reproduce the issue:

import React from 'react';
import ReactDOM from 'react-dom';
import CodeMirror from 'codemirror';
import {UnControlled} from 'react-codemirror2';

function keyDown(realCM, event) {
  if (event.key === "Backspace") {
    event.preventDefault();

    // Create a temporary CodeMirror instance                                      
    const tmpDiv = document.createElement('div');
    const tmpCM = CodeMirror(tmpDiv, {value: ""});
    tmpCM.setValue(realCM.getValue());

    // Define an on("changes") handler                                             
    let handler = (_, changeArray) => {
      console.log("!!! Handler called !!!"); // Never called!                      
    };

    // Make a change. This should trigger on("changes"), but it won't.             
    console.log("tmpCM.curOp is set before anything has happened:", tmpCM.curOp);
    tmpCM.on('changes', handler);
    tmpCM.operation(() => {
      console.log("Inside of `.operation()`. Calling `tmpCM.replaceRange`");
      tmpCM.replaceRange("", {line: 0, ch: 0}, {line: 0, ch: 4}, 'cmb:delete');
    });
    tmpCM.off('changes', handler);
  }
}

ReactDOM.render(
  (<UnControlled
      value={"test"}
      onKeyDown={keyDown}
   />),
  document.getElementById('cmb-editor'));

To reproduce, place the cursor at the end of "test", and press backspace.

The issue is that the on("changes") handler fails to fire after the call to .operation(). You can see this because console.log("!!! Handler called !!!") is never logged.

Speculation on Causes

I looked at why on("changes") fails to fire, and see two potential reasons.

  1. After the call to tmpCM.setValue(realCM.getValue()), tmpCM.curOp is not null. As a result, runInOp() (which gets invoked when we call tmpCM.operation) never calls endOperation() (which would eventually call signal()).

  2. The "changes" handler fails to fire because in the outermost stack frame, by the time the handlers are checked, tmpCM.off("changes") has already been called in the above code. (This stack frame looked like it was related to the key press, so I wouldn't expect it to be responsible for calling the "changes" handler, though.) As a demonstration of this, if you wrap the call to tmpCM.off in setTimeout(..., 0), then the issue is resolved and the "changes" handler is called.

scniro commented 4 years ago

@justinpombrio I am a lot shorter on time these days as when I started this project. Codemirror & React APIs are moving to quickly for me to keep atop of for the day-to-day. I am looking for a co-maintainer of this project. Please contact me directly if you are interested. Thank you for understanding.

justinpombrio commented 4 years ago

@scniro I don't have time to work on codemirror-react2 myself. That said, maintaining an open source project like this is a valuable public service. Thank you for your service, however much time you have to dedicate to it! It is especially admirable in the fast-paced JS ecosystem.