scniro / react-codemirror2

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

Issue w/ native CodeMirror events & React component's state #138

Open shinonomeiro opened 5 years ago

shinonomeiro commented 5 years ago

Hello, and thanks for this useful package.

I've noticed a minor issue that I believe to be a bug, regarding the way CodeMirror's native events (e.g. onBlur, onCopy, etc.) are being invoked.

Given the sample code below:

const [text, updateText] = useState('Initial text');

const handleBeforeChange = (_editor, _data, value) => {
  updateText(value);
};

const handleBlur = (editor, _e) => {
  // TODO: Save updated text to Redux store
  console.log(text);
};

return (
  <CodeMirror
    value={text}
    onBeforeChange={handleBeforeChange}
    onBlur={handleBlur}
  />
);

I'm not using the event object _e from the callback as its target.value returns any highlighted text at the time of the blur event or an empty string otherwise, which does not suit my needs (and is also a slightly strange behavior IMO, but we're getting off-topic).

You would expect handleBlur above to print the entire updated text after a few inputs, which are persisted to the component's state, but it's apparently not the case as text from inside handleBlur still points for some reason to the initial value given during the component's initialization, i.e. 'Initial text'.

Digging into the wrapper's code, the wire method on the Shared object seems to be at fault, most likely referring to a "wrong" this. As a quick fix I tried replacing this.shared.wire(this.props); with this.shared.wire.call(_this, this.props); (in componentDidMount https://github.com/scniro/react-codemirror2/blob/6.0.0/index.js#L532) giving it Controlled instead of Shared as this and it does seem to work, but I have no much clue as to why. Besides, there is a lot of this and _this juggling around which is fairly confusing.

Could somebody please look into this and possibly provide a more elegant fix? Thanks in advance.

scniro commented 4 years ago

@shinonomeiro 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.

gsouf commented 4 years ago

@scniro that's occuring because react-code-mirror2 stores the props on construct and never updates them. (https://github.com/scniro/react-codemirror2/blob/cc3a47d0707b060bf1a1e42ccb258b9b7c0882fd/src/index.tsx#L124)

A quick fix would be to change the internal prop instance everytime it updates.

A better fix would be to rebind/unbind any listener that changed when props changed.

gsouf commented 4 years ago

@shinonomeiro the other alternative is to use a class component instead of a function compnent.

That's because you would then this.state.text instead of the hook value text. Because the reference to this is permanent for the component, even if the event is not bound again, when text is a new reference everytimes.