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

fix: fix React Node View render problem in React 18 #2985

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

Darmody
Copy link
Contributor

@Darmody Darmody commented Jul 14, 2022

After switching to React 18 createRoot() API, we met the selection bug for the block using react custom node view.

https://github.com/ProseMirror/prosemirror-view/blob/master/src/viewdesc.ts#L420
When prosemirror wants to update the dom selection, if the anchorNode points to a react portal dom, it is still not mounted (anchorNode.isConnect equals false). This will cause the selection to stay in the position before when create a new block with the react node view.

For example, extending the paragraph with react node view and pressing Enter at the end of the document will reproduce this bug.

After digging the code, I think the bug comes from https://github.com/ueberdosis/tiptap/blob/main/packages/react/src/ReactRenderer.tsx#L80-L88

After React 18 updates inside of timeouts, promises, native event handlers, or any other event are batched.
https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#automatic-batching

That causes the node view portals to render in an async way.
https://github.com/ueberdosis/tiptap/blob/main/packages/react/src/EditorContent.tsx#L7-L19

So flushSync is necessary to opt-out of automatic batching
https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#automatic-batching

@netlify
Copy link

netlify bot commented Jul 14, 2022

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 4f47f77
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/62d0451821b3be00091ff529
😎 Deploy Preview https://deploy-preview-2985--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.

@coderinblack08
Copy link

coderinblack08 commented Jul 22, 2022

Outputs error in console:

Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.

@jamesopti
Copy link

Any updates here from the Tiptap team?

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.

looks good, thanks!

@bdbch bdbch merged commit f32293b into ueberdosis:main Aug 22, 2022
@essential-randomness
Copy link

Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.

Has this made it to a release? I'm still getting the error above on 2.0.0-beta.194.

@sampi
Copy link
Contributor

sampi commented Sep 12, 2022

Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.

Has this made it to a release? I'm still getting the error above on 2.0.0-beta.194.

I believe that this PR causes that error, it doesn't happen in 2.0.0-beta.114

@essential-randomness
Copy link

Indeed. Can confirm the code for this PR is there on 2.0.0-beta.194, where I'm seeing the issue above.

@bdbch
Copy link
Contributor

bdbch commented Sep 13, 2022

This should be included in 2.0.0-beta.194 yes.

sampi pushed a commit to sampi/tiptap that referenced this pull request Sep 13, 2022
To avoid seeing the `Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.` error, we need to move the `flushSync()` code that avoids automatic batching to a microtask to not fire a lifecycle event `setState()` during rendering.

Fixes warning introduced in ueberdosis#2985
bdbch pushed a commit that referenced this pull request Sep 13, 2022
To avoid seeing the `Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.` error, we need to move the `flushSync()` code that avoids automatic batching to a microtask to not fire a lifecycle event `setState()` during rendering.

Fixes warning introduced in #2985
andrewlu0 pushed a commit to trybaseplate/tiptap that referenced this pull request Oct 20, 2023
andrewlu0 pushed a commit to trybaseplate/tiptap that referenced this pull request Oct 20, 2023
To avoid seeing the `Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.` error, we need to move the `flushSync()` code that avoids automatic batching to a microtask to not fire a lifecycle event `setState()` during rendering.

Fixes warning introduced in ueberdosis#2985
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants