Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Destroy editor in safe #4000

Merged
merged 3 commits into from
Jul 7, 2023
Merged

Conversation

KentoMoriwaki
Copy link
Contributor

@KentoMoriwaki KentoMoriwaki commented Apr 30, 2023

Please describe your changes

Fix #3764

How did you accomplish your changes

There are two issues:

  • The EditorContent is reused even though the instance of the editor has changed.
  • The deletion of the editor occurs before the deletion of the EditorContent.

I solved those issues.

How have you tested your changes

I tested by changing demos/src/Examples/CustomParagraph/React/index.jsx below.

Test code
import './styles.scss'

import { EditorContent, useEditor } from '@tiptap/react'
import StarterKit from '@tiptap/starter-kit'
import React, { useEffect } from 'react'

import { Paragraph } from './Paragraph.jsx'

export default () => {
  const [data, setData] = React.useState({ content: '' })

  useEffect(() => {
    const timer = setInterval(() => {
      setData({
        content: `
      <p>
        Each line shows the number of characters in the paragraph.
      </p>
      `,
      })
    }, 5000)

    return () => clearInterval(timer)
  }, [])

  const editor = useEditor({
    extensions: [
      StarterKit.configure({
        paragraph: false,
      }),
      Paragraph,
    ],
    content: data.content,
  }, [data])

  return (
    <EditorContent editor={editor} />
  )
}

How can we verify your changes

By running the above example code on the develop branch, you will see that the flushSync error appears, but it does not appear on this branch.

Remarks

[add any additional remarks here]

Checklist

  • The changes are not breaking the editor
  • Added tests where possible
  • Followed the guidelines
  • Fixed linting issues

Related issues

[add a link to the related issues here]

@netlify
Copy link

netlify bot commented Apr 30, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 181400c
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/645b5256f8cabc000854f318
😎 Deploy Preview https://deploy-preview-4000--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@KentoMoriwaki
Copy link
Contributor Author

The intention of this change is to organize the lifecycle of the editor instance and EditorContent component.

To avoid the current issue with flushSync warning, the editor needs to be removed in the following order

  • EditorContent is unmounted
  • editor instance is destroyed

Currently, in the implementation, the sequence is as follows when the editor instance is recreated

  • Old editor instance is destroyed
  • New editor instance is created
  • EditorContent is updated

With the changes in this pull request, the sequence will be as follows, and the issue with flushSync warning will be resolved:

  • New editor instance is created
  • Old EditorContent is unmounted
  • New EditorContent is mounted
  • Old editor instance is destroyed

@KentoMoriwaki
Copy link
Contributor Author

@bdbch Can you take a look?

@Just-Moh-it
Copy link

Hey reviewers... please have a look... This issue seems to be a little problematic when using tiptap and fixing this would really help!

@nk11dev
Copy link

nk11dev commented May 25, 2023

Please, take a look at this PR, because I did not found any workaround without it.

This fix could solve the issue.

@MadsFrost
Copy link

Hey guys, is this still an issue in progress? I still have this issue appear for me on v2.0.3

Copy link
Contributor

@bdbch bdbch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks. @svenadlung what are your thoughts? Mergeable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants