-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(react): onCreate fired twice under React.StrictMode in useEditor #8095
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
1f9ca89
beef6fb
5227c55
c08706d
0a72984
6abfc27
08906c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@tiptap/react': patch | ||
| --- | ||
|
|
||
| Fix `useEditor` firing `onCreate` twice for two different `Editor` instances under React StrictMode. `useEditor`'s `useState` lazy initializer is invoked twice by StrictMode for a single mount, and both calls construct (and auto-mount) a real `Editor`, so both used to schedule their own `create` event. `onCreate` is now buffered until the owning instance is confirmed mounted (i.e. its render effect actually runs) and only forwarded then - an instance whose effect never runs (the discarded one) never forwards it. Verified against React 18.2.0, 18.3.1, and 19.1.0. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,269 @@ | ||
| import { Document } from '@tiptap/extension-document' | ||
| import { Paragraph } from '@tiptap/extension-paragraph' | ||
| import { Text } from '@tiptap/extension-text' | ||
| import { render } from '@testing-library/react' | ||
| import React from 'react' | ||
| import { afterEach, describe, expect, it } from 'vitest' | ||
|
|
||
| import type { Editor } from '@tiptap/core' | ||
| import { useEditor } from './useEditor.js' | ||
|
|
||
| function flushTimers(ms: number) { | ||
| return new Promise<void>(resolve => { | ||
| setTimeout(resolve, ms) | ||
| }) | ||
| } | ||
|
|
||
| describe('useEditor', () => { | ||
| afterEach(() => { | ||
| document.body.innerHTML = '' | ||
| }) | ||
|
|
||
| it('does not fire onCreate more than once for a single mount under StrictMode', async () => { | ||
| // onBeforeCreate legitimately still fires for both the kept and the | ||
| // discarded StrictMode-duplicate instance (it fires synchronously as | ||
| // part of construction itself, before there's any way to know which one | ||
| // survives). onCreate is what's actually fixed here: it's deferred until | ||
| // the instance is confirmed mounted, so only the kept instance's onCreate | ||
| // is ever forwarded to the user's callback. | ||
| let beforeCreateCount = 0 | ||
| let createCount = 0 | ||
| const createdEditors = new Set<Editor>() | ||
| let latestEditor: Editor | null = null | ||
|
|
||
| function TestComponent() { | ||
| const editor = useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| onBeforeCreate: () => { | ||
| beforeCreateCount += 1 | ||
| }, | ||
| onCreate: ({ editor: createdEditor }) => { | ||
| createCount += 1 | ||
| createdEditors.add(createdEditor) | ||
| }, | ||
| }) | ||
|
|
||
| latestEditor = editor | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| const { unmount } = render( | ||
| React.createElement(React.StrictMode, null, React.createElement(TestComponent)), | ||
| ) | ||
|
|
||
| // The editor's own 'create' event fires via an internal setTimeout(0); | ||
| // give it real wall-clock time to flush. | ||
| await flushTimers(100) | ||
|
|
||
| expect(beforeCreateCount).toBeGreaterThanOrEqual(1) | ||
| expect(createCount).toBe(1) | ||
| expect(createdEditors.size).toBe(1) | ||
| expect(latestEditor?.isDestroyed).toBe(false) | ||
|
|
||
| unmount() | ||
| }) | ||
|
|
||
| it('still fires onCreate exactly once when mounted without StrictMode', async () => { | ||
| let createCount = 0 | ||
|
|
||
| function TestComponent() { | ||
| useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| onCreate: () => { | ||
| createCount += 1 | ||
| }, | ||
| }) | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| const { unmount } = render(React.createElement(TestComponent)) | ||
|
|
||
| await flushTimers(100) | ||
|
|
||
| expect(createCount).toBe(1) | ||
|
|
||
| unmount() | ||
| }) | ||
|
|
||
| it('keeps two sibling editors independent under StrictMode', async () => { | ||
| let createCountA = 0 | ||
| let createCountB = 0 | ||
| const createdA = new Set<Editor>() | ||
| const createdB = new Set<Editor>() | ||
| let editorA: Editor | null = null | ||
| let editorB: Editor | null = null | ||
|
|
||
| function ComponentA() { | ||
| const editor = useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| onCreate: ({ editor: e }) => { | ||
| createCountA += 1 | ||
| createdA.add(e) | ||
| }, | ||
| }) | ||
|
|
||
| editorA = editor | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| function ComponentB() { | ||
| const editor = useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| onCreate: ({ editor: e }) => { | ||
| createCountB += 1 | ||
| createdB.add(e) | ||
| }, | ||
| }) | ||
|
|
||
| editorB = editor | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| const { unmount } = render( | ||
| React.createElement( | ||
| React.StrictMode, | ||
| null, | ||
| React.createElement(ComponentA), | ||
| React.createElement(ComponentB), | ||
| ), | ||
| ) | ||
|
|
||
| await flushTimers(100) | ||
|
|
||
| expect(createCountA).toBe(1) | ||
| expect(createCountB).toBe(1) | ||
| expect(createdA.size).toBe(1) | ||
| expect(createdB.size).toBe(1) | ||
| expect(editorA?.isDestroyed).toBe(false) | ||
| expect(editorB?.isDestroyed).toBe(false) | ||
| expect(editorA).not.toBe(editorB) | ||
|
|
||
| unmount() | ||
| }) | ||
|
|
||
| it('does not throw and still creates exactly one editor when immediatelyRender is false under StrictMode', async () => { | ||
| let createCount = 0 | ||
|
|
||
| function TestComponent() { | ||
| useEditor({ | ||
| immediatelyRender: false, | ||
| extensions: [Document, Text, Paragraph], | ||
| onCreate: () => { | ||
| createCount += 1 | ||
| }, | ||
| }) | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| let unmount: () => void = () => {} | ||
|
|
||
| expect(() => { | ||
| ;({ unmount } = render( | ||
| React.createElement(React.StrictMode, null, React.createElement(TestComponent)), | ||
| )) | ||
| }).not.toThrow() | ||
|
|
||
| await flushTimers(100) | ||
|
|
||
| expect(createCount).toBe(1) | ||
|
|
||
| unmount() | ||
| }) | ||
|
|
||
| it('handles a full unmount and remount cycle correctly under StrictMode', async () => { | ||
| // Tracks *deltas* around the real show/hide toggle, rather than an | ||
| // absolute destroyCount, so this stays correct regardless of exactly | ||
| // when the guard's ref is (re-)initialized across mounts. | ||
| let createCount = 0 | ||
| let destroyCount = 0 | ||
|
|
||
| function TestComponent() { | ||
| useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| onCreate: () => { | ||
| createCount += 1 | ||
| }, | ||
| onDestroy: () => { | ||
| destroyCount += 1 | ||
| }, | ||
| }) | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| function Wrapper({ show }: { show: boolean }) { | ||
| return show ? React.createElement(TestComponent) : null | ||
| } | ||
|
|
||
| const { rerender, unmount } = render( | ||
| React.createElement(React.StrictMode, null, React.createElement(Wrapper, { show: true })), | ||
| ) | ||
|
|
||
| await flushTimers(100) | ||
| expect(createCount).toBe(1) | ||
|
|
||
| const destroyCountBeforeHiding = destroyCount | ||
|
|
||
| rerender( | ||
| React.createElement(React.StrictMode, null, React.createElement(Wrapper, { show: false })), | ||
| ) | ||
| await flushTimers(100) | ||
| expect(destroyCount - destroyCountBeforeHiding).toBe(1) | ||
|
|
||
| rerender( | ||
| React.createElement(React.StrictMode, null, React.createElement(Wrapper, { show: true })), | ||
| ) | ||
| await flushTimers(100) | ||
| expect(createCount).toBe(2) | ||
|
|
||
| unmount() | ||
| }) | ||
|
|
||
| it('does not crash the render if a user-provided onCreate throws', async () => { | ||
| function TestComponent() { | ||
| useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| onCreate: () => { | ||
| throw new Error('boom from onCreate') | ||
| }, | ||
| }) | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| const caughtAsync: unknown[] = [] | ||
| const onUnhandled = (error: unknown) => { | ||
| caughtAsync.push(error) | ||
| } | ||
|
|
||
| process.on('uncaughtException', onUnhandled) | ||
|
|
||
| let unmount: () => void = () => {} | ||
|
|
||
| try { | ||
| expect(() => { | ||
| ;({ unmount } = render( | ||
| React.createElement(React.StrictMode, null, React.createElement(TestComponent)), | ||
| )) | ||
| }).not.toThrow() | ||
|
|
||
| await flushTimers(500) | ||
| } finally { | ||
| unmount() | ||
| await flushTimers(100) | ||
| process.off('uncaughtException', onUnhandled) | ||
| } | ||
|
|
||
| // A throwing onCreate should surface asynchronously (same as it would | ||
| // have before this fix, since it fires from a 0ms timer or a React | ||
| // effect, never synchronously inside the render call itself) - not | ||
| // crash the render. | ||
| expect(caughtAsync.length).toBeGreaterThanOrEqual(1) | ||
| expect(caughtAsync.every(error => error instanceof Error)).toBe(true) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||
| import { type EditorOptions, Editor } from '@tiptap/core' | ||||||||||||||||||||||||||
| import { type EditorEvents, type EditorOptions, Editor } from '@tiptap/core' | ||||||||||||||||||||||||||
| import type { DependencyList, MutableRefObject } from 'react' | ||||||||||||||||||||||||||
| import { useDebugValue, useEffect, useRef, useState } from 'react' | ||||||||||||||||||||||||||
| import { useSyncExternalStore } from 'use-sync-external-store/shim/index.js' | ||||||||||||||||||||||||||
|
|
@@ -59,6 +59,19 @@ class EditorInstanceManager { | |||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| private isComponentMounted = false | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Under React 18/19 StrictMode, the `useState` lazy initializer that | ||||||||||||||||||||||||||
| * constructs this manager is invoked twice for a single logical mount; the | ||||||||||||||||||||||||||
| * discarded instance's `Editor` still auto-mounts and schedules its own | ||||||||||||||||||||||||||
| * `create` event via a 0ms timer. Rather than trying to prevent that | ||||||||||||||||||||||||||
| * second construction (StrictMode's exact hook-sharing behavior differs | ||||||||||||||||||||||||||
| * across React versions, so that's not reliable), buffer a `create` event | ||||||||||||||||||||||||||
| * that fires before this instance is confirmed mounted, and only forward | ||||||||||||||||||||||||||
| * it once `onRender`'s effect actually runs. An instance whose effect | ||||||||||||||||||||||||||
| * never runs (i.e. the discarded one) simply never forwards it. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| private pendingOnCreateEvent: EditorEvents['create'] | null = null | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * The most recent dependencies array. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
|
|
@@ -124,7 +137,16 @@ class EditorInstanceManager { | |||||||||||||||||||||||||
| // Always call the most recent version of the callback function by default | ||||||||||||||||||||||||||
| onBeforeCreate: (...args) => this.options.current.onBeforeCreate?.(...args), | ||||||||||||||||||||||||||
| onBlur: (...args) => this.options.current.onBlur?.(...args), | ||||||||||||||||||||||||||
| onCreate: (...args) => this.options.current.onCreate?.(...args), | ||||||||||||||||||||||||||
| onCreate: props => { | ||||||||||||||||||||||||||
| if (this.isComponentMounted) { | ||||||||||||||||||||||||||
| this.options.current.onCreate?.(props) | ||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||
| // Not confirmed mounted yet - this may be a StrictMode duplicate | ||||||||||||||||||||||||||
| // whose onRender effect will never run. Buffer it; onRender flushes | ||||||||||||||||||||||||||
| // this if the instance turns out to be the real one. | ||||||||||||||||||||||||||
| this.pendingOnCreateEvent = props | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| onDestroy: (...args) => this.options.current.onDestroy?.(...args), | ||||||||||||||||||||||||||
| onFocus: (...args) => this.options.current.onFocus?.(...args), | ||||||||||||||||||||||||||
| onSelectionUpdate: (...args) => this.options.current.onSelectionUpdate?.(...args), | ||||||||||||||||||||||||||
|
|
@@ -222,6 +244,12 @@ class EditorInstanceManager { | |||||||||||||||||||||||||
| // Cleanup any scheduled destructions, since we are currently rendering | ||||||||||||||||||||||||||
| clearTimeout(this.scheduledDestructionTimeout) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (this.pendingOnCreateEvent) { | ||||||||||||||||||||||||||
| const event = this.pendingOnCreateEvent | ||||||||||||||||||||||||||
| this.pendingOnCreateEvent = null | ||||||||||||||||||||||||||
| this.options.current.onCreate?.(event) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+247
to
+251
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Guard against flushing If Adding a destroyed check is a one-line defensive guard: 🛡️ Proposed fix if (this.pendingOnCreateEvent) {
const event = this.pendingOnCreateEvent
this.pendingOnCreateEvent = null
- this.options.current.onCreate?.(event)
+ if (!event.editor.isDestroyed) {
+ this.options.current.onCreate?.(event)
+ }
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (this.editor && !this.editor.isDestroyed && deps.length === 0) { | ||||||||||||||||||||||||||
| // if the editor does exist & deps are empty, we don't need to re-initialize the editor generally | ||||||||||||||||||||||||||
| if (!EditorInstanceManager.compareOptions(this.options.current, this.editor.options)) { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.