RIP21 / react-simplemde-editor

React wrapper for simplemde (easymde) markdown editor
https://react-simplemde-edtior.netlify.com/
MIT License
766 stars 103 forks source link

Cannot read property 'parentNode' of null, after migrating to v5.0.1 #163

Closed gurleensethi closed 3 years ago

gurleensethi commented 3 years ago

Library Version: 5.0.1

I am using this component in the below structure:

function Parent() {
  const [text, setText] = useState("");
  const handleChange = (text) => {
    setText(text);
  }

  return <Child text={text} onChange={handleChange}/>
}

function Child({text, onChange}) {
  // Some other shenanigans

  return <SimpleMDEEditor value={text} onChange={onChange} />
}

Whenever I start typing in the editor, the application crashes and below is the stack trace that appears.

TypeError: Cannot read property 'parentNode' of null
EasyMDE.push../node_modules/easymde/src/js/easymde.js.EasyMDE.toTextArea
node_modules/easymde/src/js/easymde.js:2905
  2902 | }
  2903 | 
  2904 | // Unwrap easyMDEcontainer before codemirror toTextArea() call
> 2905 | easyMDEContainer.parentNode.insertBefore(wrapper, easyMDEContainer);
       | ^  2906 | easyMDEContainer.remove();
  2907 | 
  2908 | cm.toTextArea();
View compiled
(anonymous function)
src/SimpleMdeReact.tsx:176
  173 | // if this effect is getting called again means options has changed hence old instance shall be removed
  174 | // ref used to avoid endless loop
  175 | if (editorRef.current) {
> 176 |   editorRef.current?.toTextArea();
      | ^  177 |   // @ts-expect-error
  178 |   editorRef.current?.cleanup();
  179 | }
View compiled
▼ 16 stack frames were expanded.
invokePassiveEffectCreate
node_modules/react-dom/cjs/react-dom.development.js:23487
HTMLUnknownElement.callCallback
node_modules/react-dom/cjs/react-dom.development.js:3945
invokeGuardedCallbackDev
node_modules/react-dom/cjs/react-dom.development.js:3994
invokeGuardedCallback
node_modules/react-dom/cjs/react-dom.development.js:4056
flushPassiveEffectsImpl
node_modules/react-dom/cjs/react-dom.development.js:23574
unstable_runWithPriority
node_modules/scheduler/cjs/scheduler.development.js:468
runWithPriority$1
node_modules/react-dom/cjs/react-dom.development.js:11276
flushPassiveEffects
node_modules/react-dom/cjs/react-dom.development.js:23447
performSyncWorkOnRoot
node_modules/react-dom/cjs/react-dom.development.js:22269
(anonymous function)
node_modules/react-dom/cjs/react-dom.development.js:11327
unstable_runWithPriority
node_modules/scheduler/cjs/scheduler.development.js:468
runWithPriority$1
node_modules/react-dom/cjs/react-dom.development.js:11276
flushSyncCallbackQueueImpl
node_modules/react-dom/cjs/react-dom.development.js:11322
flushSyncCallbackQueue
node_modules/react-dom/cjs/react-dom.development.js:11309
scheduleUpdateOnFiber
node_modules/react-dom/cjs/react-dom.development.js:21893
dispatchAction
node_modules/react-dom/cjs/react-dom.development.js:16139
▲ 16 stack frames were expanded.
_callee$
src/SimpleMdeReact.tsx:189
  186 | const imageUploadFunction = options?.imageUploadFunction
  187 |   ? imageUploadCallback
  188 |   : undefined;
> 189 | setEditor(
      | ^  190 |   new SimpleMDE(
  191 |     Object.assign({}, initialOptions, options, {
  192 |       imageUploadFunction,
View compiled
▼ 5 stack frames were expanded.
tryCatch
node_modules/regenerator-runtime/runtime.js:63
Generator.invoke [as _invoke]
node_modules/regenerator-runtime/runtime.js:293
Generator.next
node_modules/regenerator-runtime/runtime.js:118
asyncGeneratorStep
node_modules/@babel/runtime/helpers/asyncToGenerator.js:3
_next
node_modules/@babel/runtime/helpers/asyncToGenerator.js:25
▲ 5 stack frames were expanded.
This screen is visible only in development. It will not appear if the app crashes in production.
Open your browser’s developer console to further inspect this error.  Click the 'X' or hit ESC to dismiss this message.

PS: This was working fine in 4.1.5

RIP21 commented 3 years ago

I'm pretty sure that example is not complete. As it works just fine in the demo with the same code.

RIP21 commented 3 years ago

https://codesandbox.io/s/still-sea-pypbe?file=/src/App.tsx Take a look here. All works fine.

markthiessen commented 3 years ago

I'm getting the same error in my app when I pass in options, like:

<SimpleMDE
    value={value}
    onChange={onChange}
    options={{ hideIcons: ["image", "side-by-side", "fullscreen", "guide"], status: false, minHeight: "100px" }}
  />

When I remove options, it doesn't crash.

markthiessen commented 3 years ago

So, after looking into this a bit more, if I pass in my options as a constant I no longer get the error. My error was likely caused by the useEffect dependency array here -> https://github.com/RIP21/react-simplemde-editor/blob/6b712e51f1e64cf9db2be1776db3e405ba09d99c/src/SimpleMdeReact.tsx#L197 If a new options object ref is passed in on render, the useEffect is triggered and setEditor gets called again, recreating the instance.

RIP21 commented 3 years ago

@markthiessen it's intentional and there is no way to get rid of that if I want options to change. Unfortunately, there is no .setOption or smth similar in MDE API, so only recreation is an option to change them reactively. https://github.com/RIP21/react-simplemde-editor#options The fact that options shall be memoized and why is described here.

RIP21 commented 3 years ago

So, meaning, there is nothing I can do nor change. And it's just integration got improved since old version and became more correct to what one can expect from it. The only way to fix this that I can see is to save the cursor position of the old instance and then when a new one is getting created to set it back into a new instance, but I guess there will be a lot of issues with that and I don't think it worth it as it is fixable by a simple memo.

markthiessen commented 3 years ago

@RIP21 for sure -- in my scenario ensuring the options are constant is a better approach anyway. It just took me a bit longer than expected to migrate from 4.x.x -> 5.0.0 as I didn't see anything standing out from the release notes, so I needed to dive in to find out why it was erroring. You have good examples in the README, so I wouldn't expect new users to run into it, more just during upgrades.

RIP21 commented 3 years ago

@markthiessen yup. Sorry for that. That's why I slightly changed a readme after multiple people encountered that, to ensure people will note that. Even old users. Thanks for these issues :) It helps improve experience for next users :)