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: Queue flushSync call #3533

Merged
merged 1 commit into from
Feb 18, 2023
Merged

fix: Queue flushSync call #3533

merged 1 commit into from
Feb 18, 2023

Conversation

kylealwyn
Copy link
Contributor

@kylealwyn kylealwyn commented Dec 13, 2022

Do the same thing as #3188 - confirmed this got rid of the warning locally

@netlify
Copy link

netlify bot commented Dec 13, 2022

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 3d0fff9
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/6398229b2d07aa00085119a3
😎 Deploy Preview https://deploy-preview-3533--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.

@kylealwyn kylealwyn changed the title Queue flushSync microtask fix: Queue flushSync microtask Dec 13, 2022
@kylealwyn kylealwyn changed the title fix: Queue flushSync microtask fix: Queue flushSync call Dec 14, 2022
@develink
Copy link

Looks good. Can you merge ASAP?

Copy link

@konstantinmuenster konstantinmuenster left a comment

Choose a reason for hiding this comment

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

Using queueMicrotask removes the warning but introduces this issue #3331 again. If we queue the update, changes are not immediately flushed to the DOM.

I think we instead need to reset the initialized flag properly when the editor unmounts. For us, the warning only occurred in those situations. Adding this.initialized = false before we reset the node views, worked for us:

  componentWillUnmount() {
    const { editor } = this.props

    if (!editor) {
      return
    }

    this.initialized = false

    if (!editor.isDestroyed) {
      editor.view.setProps({
        nodeViews: {},
      })
    }

    editor.contentComponent = null

    if (!editor.options.element.firstChild) {
      return
    }

    const newElement = document.createElement('div')

    newElement.append(...editor.options.element.childNodes)

    editor.setOptions({
      element: newElement,
    })
  }

@diegoulloao
Copy link

diegoulloao commented Dec 30, 2022

What's the status for this PR? I need this fix ASAP.

@jamesopti
Copy link

Using queueMicrotask removes the warning but introduces this issue #3331 again. If we queue the update, changes are not immediately flushed to the DOM.

@konstantinmuenster is right. Reintroducing #3331 would be a mistake.

I will try @konstantinmuenster 's fix in our fork and report back.

@Aldredcz
Copy link

Aldredcz commented Jan 9, 2023

Also experiencing this bug since we updated to 2.0.0-beta.209.
It happens when editor component is unmounted.

+1 on fixing this ASAP 🙏

@diegoulloao
Copy link

any news on this??

@JosephHalter
Copy link

+1

@g-bastianelli
Copy link

ANy news on this one? It's pretty annoying with Suspense

@anton-liubushkin
Copy link
Contributor

🤔

@james-william-r
Copy link
Contributor

james-william-r commented Jan 20, 2023

+1, my project is failing build with this issue. 😅

@JosephHalter
Copy link

+1, my project is failing build with this issue. 😅

Rolling back to 2.0.0-beta.202 is the only solution right now.

@amit-verma-qatalog
Copy link

upgraded to 2.0.0-beta.209 and i see a lot of these errors. is there any timeline for this fix, or should we revert to 202

@kalda341
Copy link

@jamesopti, how did that fix go? Would be great to get this sorted.

@jamesopti
Copy link

jamesopti commented Feb 25, 2023

@jamesopti, how did that fix go? Would be great to get this sorted.

Sorry for the delay in testing this. I just got a chance to test this PR and unfortunately, the issue NodeView cursor issue remains.

Here is a codesandbox showing the issue with the latest code from @tiptap/react manually included (which includes this commit)

https://codesandbox.io/s/react-nodeview-flushsync-debug-mji07b?file=/src/App.tsx

@jamesopti
Copy link

Here's a codesandbox using @konstantinmuenster 's fix and it appears to work as expected: https://codesandbox.io/s/react-nodeview-flushsync-debug-alternate-thmr3m?file=/src/App.tsx

I'll test this in our app and report back.

@bdbch
Copy link
Member

bdbch commented Feb 26, 2023

Hey. I have the fix proposed by @konstantinmuenster open as a PR already and will release it on monday morning. Looking forward to hear your feedback!

bdbch pushed a commit that referenced this pull request Mar 27, 2023
* Add custom paragraph example

* Remove unnecessary queueMicrotask
@maxenko
Copy link

maxenko commented Apr 1, 2023

2.0.1
This bug is still there. Happens what appears to be about 70% of the time when updating custom properties.

triggered via either of the commands:

addCommands() {
	return {
		setInlineImageFloat: (floatTo) => ({ commands }) => {
			return commands.updateAttributes('inline-image', {'data-float': floatTo});
		},

		setInlineImageSize: (sizeTo) => ({ commands }) => {
			return commands.updateAttributes('inline-image', {'data-size': sizeTo});
		}
	};
},
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.
ImageComponent@webpack-internal:///./src/components/ui/kit/editor/tiptap/InlineImage/index.js:75:54
ReactNodeView
Portals@webpack-internal:///./node_modules/@tiptap/react/dist/index.js:143:17
PureEditorContent@webpack-internal:///./node_modules/@tiptap/react/dist/index.js:150:9
div
__webpack_exports__.default<@webpack-internal:///./src/components/ui/kit/editor/tiptap/tiptap-editor.js:109:75
div
main
IndexPage@webpack-internal:///./src/pages/[...].js?export=default:68:30
PageRenderer@webpack-internal:///./.cache/page-renderer.js:24:13
PageQueryStore@webpack-internal:///./.cache/query-result-store.js:35:30
RouteHandler
div
re@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:9911
ee@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:9706
ae@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:11013
oe@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:10831
ScrollHandler@webpack-internal:///./node_modules/gatsby-react-router-scroll/scroll-handler.js:23:35
RouteUpdates@webpack-internal:///./.cache/navigation.js:253:32
EnsureResources@webpack-internal:///./.cache/ensure-resources.js:19:30
LocationHandler@webpack-internal:///./.cache/root.js:62:29
G@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:9063
Root
ye@webpack-internal:///./node_modules/styled-components/dist/styled-components.browser.esm.js:29:12744
QueryParamProviderInner@webpack-internal:///./node_modules/use-query-params/dist/QueryParamProvider.js:27:33
QueryParamProvider@webpack-internal:///./node_modules/use-query-params/dist/QueryParamProvider.js:46:28
z<@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:8273
F@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:7181
H@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:7472
WithErrorBoundary()
G@webpack-internal:///./node_modules/@gatsbyjs/reach-router/dist/index.modern.mjs:36:9063
__webpack_exports__.default@webpack-internal:///./node_modules/gatsby-plugin-use-query-params/root.js:17:7
StaticQueryStore@webpack-internal:///./.cache/query-result-store.js:130:32
SliceDataStore@webpack-internal:///./.cache/query-result-store.js:177:32
ErrorBoundary@webpack-internal:///./.cache/fast-refresh-overlay/components/error-boundary.js:21:35
DevOverlay@webpack-internal:///./.cache/fast-refresh-overlay/index.js:120:7
RootWrappedWithOverlayAndProvider
App@webpack-internal:///./.cache/app.js:167:80

aliasliao pushed a commit to aliasliao/tiptap that referenced this pull request May 24, 2023
aliasliao pushed a commit to aliasliao/tiptap that referenced this pull request May 24, 2023
…berdosis#3533 (ueberdosis#3862)

* Add custom paragraph example

* Remove unnecessary queueMicrotask
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
…berdosis#3533 (ueberdosis#3862)

* Add custom paragraph example

* Remove unnecessary queueMicrotask
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.