-
-
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 1 commit
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. StrictMode invokes the hook's internal lazy initializer twice, and both calls used to construct and auto-mount a real `Editor`; the discarded instance is now destroyed synchronously, before its `create` event has a chance to fire. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,231 @@ | ||
| 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 () => { | ||
| let createCount = 0 | ||
| const createdEditors = new Set<Editor>() | ||
| let latestEditor: Editor | null = null | ||
|
|
||
| function TestComponent() { | ||
| const editor = useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| 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(createCount).toBe(1) | ||
| expect(createdEditors.size).toBe(1) | ||
| expect(latestEditor?.isDestroyed).toBe(false) | ||
|
|
||
| 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 () => { | ||
| // Note: a StrictMode mount's discarded duplicate instance is destroyed | ||
| // (and so fires its own onDestroy) regardless of this fix - that already | ||
| // happened before this fix too, just 1ms later instead of synchronously. | ||
| // This test tracks *deltas* around the real show/hide toggle so that | ||
| // pre-existing, orthogonal onDestroy noise from StrictMode's duplicate | ||
| // doesn't get mistaken for a real unmount. | ||
| 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 callback throws while evicting a discarded StrictMode duplicate', async () => { | ||
| function TestComponent() { | ||
| useEditor({ | ||
| extensions: [Document, Text, Paragraph], | ||
| onDestroy: () => { | ||
| throw new Error('boom from onDestroy') | ||
| }, | ||
| }) | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| let caughtAsync: unknown = null | ||
| const onUnhandled = (error: unknown) => { | ||
| caughtAsync = error | ||
| } | ||
|
|
||
| process.on('uncaughtException', onUnhandled) | ||
|
|
||
| try { | ||
| expect(() => { | ||
| render(React.createElement(React.StrictMode, null, React.createElement(TestComponent))) | ||
| }).not.toThrow() | ||
|
|
||
| await flushTimers(100) | ||
| } finally { | ||
| process.off('uncaughtException', onUnhandled) | ||
| } | ||
|
|
||
| // The throw is still surfaced (matching pre-existing behavior for a | ||
| // throwing onDestroy) - it just must not happen synchronously inside | ||
| // React's render phase. | ||
| expect(caughtAsync).toBeInstanceOf(Error) | ||
| expect((caughtAsync as Error).message).toBe('boom from onDestroy') | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,20 @@ export type UseEditorOptions = Partial<EditorOptions> & { | |
| shouldRerenderOnTransaction?: boolean | ||
| } | ||
|
|
||
| /** | ||
| * React 18/19 StrictMode invokes a `useState` lazy initializer twice for a | ||
| * single logical mount, keeping only the first call's result - but both | ||
| * calls synchronously construct (and auto-mount) a real `Editor`. Tracks, | ||
| * per `useEditor()` call (keyed by its stable options ref), the most recent | ||
| * `EditorInstanceManager` still waiting to be confirmed as mounted, so a | ||
| * duplicate construction can destroy itself before its `Editor`'s `onCreate` | ||
| * (scheduled via a 0ms timer) fires for an instance nobody will use. | ||
| */ | ||
| const unconfirmedInstanceManagers = new WeakMap< | ||
| MutableRefObject<UseEditorOptions>, | ||
| EditorInstanceManager | ||
| >() | ||
|
|
||
| /** | ||
| * This class handles the creation, destruction, and re-creation of the editor instance. | ||
| */ | ||
|
|
@@ -73,7 +87,31 @@ class EditorInstanceManager { | |
| this.options = options | ||
| this.subscriptions = new Set<() => void>() | ||
| this.setEditor(this.getInitialEditor()) | ||
| this.scheduleDestroy() | ||
|
|
||
| const unconfirmedInstance = unconfirmedInstanceManagers.get(options) | ||
|
|
||
| if (unconfirmedInstance && !unconfirmedInstance.isComponentMounted) { | ||
| // We're a duplicate construction from StrictMode's double-invoked lazy | ||
| // initializer. React keeps the first call's result, so this instance | ||
| // is guaranteed to be discarded - destroy it synchronously, before | ||
| // its Editor's 0ms-scheduled 'create' emission has a chance to fire. | ||
| try { | ||
| this.editor?.destroy() | ||
| } catch (error) { | ||
| // A user callback (e.g. onDestroy) threw. Don't let that propagate | ||
| // through React's render phase - surface it asynchronously instead, | ||
| // same as it would have surfaced before this instance was destroyed | ||
| // synchronously (e.g. via the scheduleDestroy timer below). | ||
| setTimeout(() => { | ||
| throw error | ||
| }) | ||
| } finally { | ||
| this.editor = null | ||
| } | ||
|
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 | 🟠 Major | ⚡ Quick win Finish cleanup when
Please keep a local editor reference and attempt 🤖 Prompt for AI Agents |
||
| } else { | ||
| unconfirmedInstanceManagers.set(options, this) | ||
| this.scheduleDestroy() | ||
| } | ||
|
|
||
| this.getEditor = this.getEditor.bind(this) | ||
| this.getServerSnapshot = this.getServerSnapshot.bind(this) | ||
|
|
@@ -219,6 +257,9 @@ class EditorInstanceManager { | |
| // The returned callback will run on each render | ||
| return () => { | ||
| this.isComponentMounted = true | ||
| // We've been confirmed as the real instance for this hook call - no | ||
| // longer relevant to the StrictMode-duplicate detection above. | ||
| unconfirmedInstanceManagers.delete(this.options) | ||
| // Cleanup any scheduled destructions, since we are currently rendering | ||
| clearTimeout(this.scheduledDestructionTimeout) | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.