diff --git a/.changeset/lexical-update-listener-errors.md b/.changeset/lexical-update-listener-errors.md new file mode 100644 index 00000000000..0a6cee29486 --- /dev/null +++ b/.changeset/lexical-update-listener-errors.md @@ -0,0 +1,9 @@ +--- +'lexical': patch +--- + +[fix] Continue notifying update listeners after listener or cleanup errors + +As before, a listener whose own cleanup throws is skipped for that commit. Updates queued by update listeners run after a listener error without waiting for another external update. An update listener's previous cleanup is not repeated when the cleanup throws, the listener throws on its next call, or the listener unregisters itself. + +Verify: Register a throwing update listener before a history or collaboration listener. Later listeners must receive the same committed state, the first error must still reach the caller, and `onError` remains unused for listener errors. Queue an update from a later listener and verify it commits without another external update. diff --git a/packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts b/packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts index 8c0645f4da4..9e028a5fb21 100644 --- a/packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts +++ b/packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts @@ -315,35 +315,4 @@ describe('the binding writes every committed change', () => { 'paragraph("t1z t2z t6z t9z") / paragraph("tail line")', ); }); - - // Open, #9514 (not caused by an update that throws): Lexical stops - // notifying a commit's update listeners when one of them throws, so a - // binding registered after the listener that threw never writes that - // commit. - test.fails( - "a commit whose update listeners stopped before the binding's is not written (#9514)", - async () => { - let armed = false; - const t = await setup($doc, { - prepare: (editor: LexicalEditor) => { - editor.registerUpdateListener(() => { - if (armed) { - armed = false; - throw new Error('listener bug'); - } - }); - }, - }); - armed = true; - expect(() => - t.a.update(() => void $selectWords('t2z').removeText()), - ).toThrow('listener bug'); - t.a.update(() => void $selectIn([1], 9).insertText('!')); - await t.sync(); - await expectConverged( - t.peers, - 'paragraph("t1z t3z t4z t5z t6z t7z t8z t9z") / paragraph("tail line!")', - ); - }, - ); }); diff --git a/packages/lexical-y/src/__tests__/unit/regressions/ThrowingUpdateListener.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/ThrowingUpdateListener.test.ts new file mode 100644 index 00000000000..b76c5eccd29 --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/ThrowingUpdateListener.test.ts @@ -0,0 +1,70 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ + +// @vitest-environment node + +import {$getRoot, type LexicalEditor} from 'lexical'; +import {describe, expect, test} from 'vitest'; + +import { + $block, + $p, + $selectIn, + expectConverged, + setup, +} from '../../utils/harness'; + +// #9514: a throwing application listener registered before the binding must +// not prevent the binding from writing an already committed delete. +describe.each(['reports', 'rethrows'] as const)( + 'a throwing update listener, onError %s', + mode => { + test('the binding writes the delete and every later edit', async () => { + let armed = false; + const reported: Error[] = []; + const failure = new Error('listener bug'); + const t = await setup( + () => + $getRoot().append( + $p('t1z t2z t3z t4z t5z t6z t7z t8z t9z'), + $p('tail line'), + ), + { + prepare: (editor: LexicalEditor) => { + editor._onError = error => { + reported.push(error); + if (mode === 'rethrows') { + throw error; + } + }; + editor.registerUpdateListener(() => { + if (armed) { + armed = false; + throw failure; + } + }); + }, + }, + ); + armed = true; + expect(() => + t.a.update(() => { + const at = $block([0]).getTextContent().indexOf('t2z'); + $selectIn([0], at, at + 3).removeText(); + }), + ).toThrow(failure); + t.a.update(() => void $selectIn([1], 9).insertText('!')); + await t.sync(); + await expectConverged( + t.peers, + 'paragraph("t1z t3z t4z t5z t6z t7z t8z t9z") / paragraph("tail line!")', + ); + expect(reported).toEqual([]); + }); + }, +); diff --git a/packages/lexical/src/LexicalUpdates.ts b/packages/lexical/src/LexicalUpdates.ts index 053a62a9300..661dd3a9c1a 100644 --- a/packages/lexical/src/LexicalUpdates.ts +++ b/packages/lexical/src/LexicalUpdates.ts @@ -938,6 +938,7 @@ export function triggerListeners( isCurrentlyEnqueuingUpdates: boolean, ...payload: MapListeners[T] ): void { + let rethrown: null | {error: unknown} = null; const previouslyUpdating = editor._updating; editor._updating = isCurrentlyEnqueuingUpdates; @@ -948,23 +949,44 @@ export function triggerListeners( >; const listeners = Array.from(listenerMap); for (const [listener, unregister] of listeners) { - if (unregister) { - unregister(); - } - // TypeScript's void-return rule lets a `=> void` callback return any value, - // so a listener like `() => arr.push(x)` hands back a number. Only a - // function is an unregister callback. - const result = listener(...payload); - const nextUnregister = typeof result === 'function' ? result : undefined; - if (listenerMap.has(listener)) { - listenerMap.set(listener, nextUnregister); - } else if (nextUnregister) { - nextUnregister(); + try { + if (unregister) { + if (type === 'update' && listenerMap.has(listener)) { + // A cleanup that throws, or whose next listener throws, has still + // been called. Do not call it again on the next commit. + listenerMap.set(listener, undefined); + } + unregister(); + } + // TypeScript's void-return rule lets a `=> void` callback return any value, + // so a listener like `() => arr.push(x)` hands back a number. Only a + // function is an unregister callback. + const result = listener(...payload); + const nextUnregister = + typeof result === 'function' ? result : undefined; + if (listenerMap.has(listener)) { + listenerMap.set(listener, nextUnregister); + } else if (nextUnregister) { + nextUnregister(); + } + } catch (error) { + if (type !== 'update') { + throw error; + } + // Every update listener must see the committed state, even if an + // earlier listener throws. Keep the existing error contract: callers + // receive the first error and onError is not called for listeners. + rethrown ??= {error}; } } } finally { editor._updating = previouslyUpdating; } + if (rethrown !== null) { + // A listener may have queued updates before the commit was interrupted. + scheduleMicroTask(() => $triggerEnqueuedUpdates(editor)); + throw rethrown.error; + } } function hasSelectionChanged( diff --git a/packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts b/packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts new file mode 100644 index 00000000000..3ad9b8d0ab2 --- /dev/null +++ b/packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts @@ -0,0 +1,282 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ + +// @vitest-environment node + +import { + $createParagraphNode, + $createTextNode, + $getRoot, + createEditor, + type LexicalEditor, + type TextNode, +} from 'lexical'; +import {describe, expect, test} from 'vitest'; + +function setup(rethrows = true) { + const errors: Error[] = []; + const editor = createEditor({ + onError: error => { + errors.push(error); + if (rethrows) { + throw error; + } + }, + }); + editor.update( + () => { + $getRoot().append( + $createParagraphNode().append($createTextNode('before')), + ); + }, + {discrete: true}, + ); + return {editor, errors}; +} + +function commit(editor: LexicalEditor, text: string) { + editor.update( + () => { + ($getRoot().getFirstDescendant() as TextNode).setTextContent(text); + }, + {discrete: true}, + ); +} + +describe('update listener error isolation', () => { + test.each([true, false])( + 'later listeners see the commit and the next update, onError rethrows: %s', + rethrows => { + const {editor, errors} = setup(rethrows); + const failure = new Error('listener bug'); + let armed = true; + editor.registerUpdateListener(() => { + if (armed) { + armed = false; + throw failure; + } + }); + const commits: [string, string][] = []; + editor.registerUpdateListener(({editorState, prevEditorState}) => { + commits.push([ + prevEditorState.read(() => $getRoot().getTextContent()), + editorState.read(() => $getRoot().getTextContent()), + ]); + }); + + expect(() => commit(editor, 'after')).toThrow(failure); + expect(commits).toEqual([['before', 'after']]); + expect(editor._updating).toBe(false); + expect(editor._pendingEditorState).toBe(null); + expect(errors).toEqual([]); + + commit(editor, 'next'); + expect(commits).toEqual([ + ['before', 'after'], + ['after', 'next'], + ]); + }, + ); + + test('keeps the first of multiple listener errors', () => { + const {editor, errors} = setup(); + const first = new Error('first'); + const seen: string[] = []; + editor.registerUpdateListener(() => { + seen.push('first'); + throw first; + }); + editor.registerUpdateListener(() => { + seen.push('second'); + throw new Error('second'); + }); + editor.registerUpdateListener(() => { + seen.push('last'); + }); + + expect(() => commit(editor, 'after')).toThrow(first); + expect(seen).toEqual(['first', 'second', 'last']); + expect(errors).toEqual([]); + expect(editor._updating).toBe(false); + }); + + test.each([undefined, null, 0])( + 'a non-Error throw (%s) is preserved and does not stop notification', + failure => { + const {editor, errors} = setup(); + let notified = false; + editor.registerUpdateListener(() => { + throw failure; + }); + editor.registerUpdateListener(() => { + notified = true; + throw new Error('later error'); + }); + let thrown = false; + try { + commit(editor, 'after'); + } catch (error) { + thrown = true; + expect(error).toBe(failure); + } + expect(thrown).toBe(true); + expect(notified).toBe(true); + expect(errors).toEqual([]); + expect(editor._updating).toBe(false); + }, + ); + + test('does not repeat a throwing cleanup', () => { + const {editor} = setup(); + const failure = new Error('cleanup bug'); + let cleanupCalls = 0; + let notified = 0; + let armed = false; + editor.registerUpdateListener(() => { + return () => { + cleanupCalls++; + if (armed) { + throw failure; + } + }; + }); + editor.registerUpdateListener(() => { + notified++; + }); + + commit(editor, 'one'); + armed = true; + expect(() => commit(editor, 'two')).toThrow(failure); + expect(notified).toBe(2); + armed = false; + commit(editor, 'three'); + expect(cleanupCalls).toBe(1); + expect(notified).toBe(3); + }); + + test('does not repeat cleanup after a listener throws', () => { + const {editor} = setup(); + let cleanupCalls = 0; + let armed = false; + editor.registerUpdateListener(() => { + if (armed) { + armed = false; + throw new Error('listener bug'); + } + return () => { + cleanupCalls++; + }; + }); + + commit(editor, 'one'); + armed = true; + expect(() => commit(editor, 'two')).toThrow('listener bug'); + commit(editor, 'three'); + expect(cleanupCalls).toBe(1); + }); + + test('editable listeners still stop at the first error', () => { + const {editor, errors} = setup(); + const failure = new Error('editable listener bug'); + let notified = false; + editor.registerEditableListener(() => { + throw failure; + }); + editor.registerEditableListener(() => { + notified = true; + }); + + expect(() => editor.setEditable(!editor.isEditable())).toThrow(failure); + expect(notified).toBe(false); + expect(errors).toEqual([]); + expect(editor._updating).toBe(false); + }); + + test.each([1, 2])( + 'cleans up a listener that unregisters itself on call %s', + unregisterOn => { + const {editor} = setup(); + let cleanups = 0; + let calls = 0; + let remove = () => {}; + remove = editor.registerUpdateListener(() => { + calls++; + if (calls === unregisterOn) { + remove(); + } + return () => { + cleanups++; + }; + }); + + commit(editor, 'one'); + expect(calls).toBe(1); + expect(cleanups).toBe(unregisterOn === 1 ? 1 : 0); + commit(editor, 'two'); + expect(calls).toBe(unregisterOn); + expect(cleanups).toBe(unregisterOn); + commit(editor, 'three'); + expect(calls).toBe(unregisterOn); + expect(cleanups).toBe(unregisterOn); + }, + ); + + test.each([true, false])( + 'queued updates run after a listener error, onError rethrows: %s', + async rethrows => { + const {editor, errors} = setup(rethrows); + const failure = new Error('listener bug'); + editor.registerUpdateListener(({editorState}) => { + if (editorState.read(() => $getRoot().getTextContent()) === 'after') { + throw failure; + } + }); + let queued = false; + let queuedCalls = 0; + editor.registerUpdateListener(({editorState}) => { + if ( + !queued && + editorState.read(() => $getRoot().getTextContent()) === 'after' + ) { + queued = true; + editor.update( + () => { + queuedCalls++; + ($getRoot().getFirstDescendant() as TextNode).setTextContent( + 'queued', + ); + }, + {discrete: true}, + ); + } + }); + let thrown: unknown; + try { + commit(editor, 'after'); + } catch (error) { + thrown = error; + } + expect(thrown).toBe(failure); + expect(queuedCalls).toBe(0); + expect(editor._updating).toBe(false); + + await Promise.resolve(); + const text = editor.getEditorState().read(() => { + return $getRoot().getTextContent(); + }); + expect(text).toBe('queued'); + expect(queuedCalls).toBe(1); + expect(editor._updates).toHaveLength(0); + expect(editor._pendingEditorState).toBe(null); + expect(editor._updating).toBe(false); + expect(errors).toEqual([]); + await Promise.resolve(); + expect(queuedCalls).toBe(1); + }, + ); +});