[lexical] Bug Fix: only root and editable listeners may return a cleanup function - #8990
Conversation
…nup function
## Description
`registerRootListener` / `registerEditableListener` document that a listener may
return a cleanup function:
```ts
/**
* ... If this callback returns a function,
* that function will be called before the next value update or unregister.
*/
export type RootListener = (...) => void | (() => void);
export type EditableListener = (editable: boolean) => void | (() => void);
```
`triggerListeners` serves five listener maps, and it applied that protocol to
all of them:
```ts
const nextUnregister = listener(...payload);
if (listenerMap.has(listener)) {
listenerMap.set(listener, nextUnregister); // stores ANY return value
}
```
`UpdateListener`, `DecoratorListener` and `TextContentListener` are typed
`=> void` and their JSDoc says nothing about a cleanup return. TypeScript's
void-return rule lets any return type be assigned to a `=> void` signature, so
perfectly ordinary listeners return values:
`({editorState}) => states.push(editorState)` returns a number, an `async`
listener returns a Promise. That value was stored as the listener's
"unregister" and called on the next trigger:
```
TypeError: unregister is not a function
❯ triggerListeners packages/lexical/src/LexicalUpdates.ts:866:9
❯ $commitPendingUpdatesImpl packages/lexical/src/LexicalUpdates.ts:792:3
```
`triggerListeners` only has `try/finally`, and neither it nor
`$commitPendingUpdatesImpl` catches, so the throw escapes past
`triggerDeferredUpdateCallbacks` and `$triggerEnqueuedUpdates` — `$onUpdate`
callbacks never fire and the queued-update pump never drains. The editor stalls
rather than just logging an error. `unregisterListener` has the same
`if (unregister) unregister()` shape, so the teardown function throws too.
Restore the scope the feature was introduced with (facebook#8219, titled "LexicalEditor
RootListener and EditableListener can return unregister callbacks"): keep a
returned callback only for the two maps that document it, and only when it
really is a function.
## Test plan
Three new cases in
`packages/lexical/src/__tests__/unit/LexicalEditorListener.test.ts`, next to the
existing root/editable cleanup-return tests, which still pass unchanged.
### Before
Verified by restoring `LexicalUpdates.ts` to its pre-fix contents with the new
tests in place:
```
$ npx vitest run packages/lexical/src/__tests__/unit/LexicalEditorListener.test.ts
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 3 ⎯⎯⎯⎯⎯⎯⎯
FAIL |unit| .../LexicalEditorListener.test.ts > LexicalEditor listeners > listeners that do not opt into the cleanup return > an update listener may return a non-function value
TypeError: unregister is not a function
FAIL |unit| .../LexicalEditorListener.test.ts > LexicalEditor listeners > listeners that do not opt into the cleanup return > a text content listener may return a non-function value
TypeError: unregister is not a function
FAIL |unit| .../LexicalEditorListener.test.ts > LexicalEditor listeners > listeners that do not opt into the cleanup return > an update listener returning a function is not called back
Tests 3 failed | 6 passed (9)
```
### After
```
$ npx vitest run packages/lexical/src
Test Files 65 passed (65)
Tests 1350 passed | 1 skipped (1351)
$ npx vitest run packages/lexical-react packages/lexical-history packages/lexical-yjs packages/lexical-playground
Test Files 55 passed (55)
Tests 556 passed (556)
```
|
@LeSingh1 is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
etrepum
left a comment
There was a problem hiding this comment.
I think this goes a bit too far, would probably be better to simply add the type check to see if it's a function and leave it at that. Maybe explicitly widen the types for the other map listeners to allow for cleanup functions even though they're not all particularly useful.
…eanup ## Description `triggerListeners` stores whatever a listener returns and calls it before the next dispatch and on unregister. Only `RootListener` and `EditableListener` document that protocol; the other maps are typed `=> void`, and TypeScript's void-return rule lets a `=> void` callback return any value. So `registerUpdateListener(() => arr.push(x))` is legal, returns a number, and the next commit throws `TypeError: unregister is not a function` out of the update pump. The stored value is now taken only when it is a function, which leaves the documented cleanup protocol working for every map and makes an incidental return value harmless. Fixes facebook#8990 ## Test plan `LexicalEditorListener.test.ts` covers an update listener and a text content listener returning a non-function value, and an update listener whose returned function is used as cleanup. ### Before ``` $ npx vitest run packages/lexical/src/__tests__/unit/LexicalEditorListener.test.ts TypeError: unregister is not a function ``` ### After ``` $ npx vitest run packages/lexical/src/__tests__ packages/lexical-react packages/lexical-history Test Files 89 passed (89) Tests 1143 passed | 1 skipped (1144) ```
|
Narrowed it to just the type check, as you suggested — the allow-list is gone, so the documented cleanup protocol now works for every listener map and only a function is ever stored as the unregister callback. On explicitly widening the other listener types: I tried it and it is a breaking change, so I have left it out. Going from Any downstream
|
Description
registerRootListener/registerEditableListenerdocument that a listener mayreturn a cleanup function:
triggerListenersserves five listener maps, and it applied that protocol toall of them:
UpdateListener,DecoratorListenerandTextContentListenerare typed=> voidand their JSDoc says nothing about a cleanup return. TypeScript'svoid-return rule lets any return type be assigned to a
=> voidsignature, soperfectly ordinary listeners return values:
({editorState}) => states.push(editorState)returns a number, anasynclistener returns a Promise. That value was stored as the listener's
"unregister" and called on the next trigger:
triggerListenersonly hastry/finally, and neither it nor$commitPendingUpdatesImplcatches, so the throw escapes pasttriggerDeferredUpdateCallbacksand$triggerEnqueuedUpdates—$onUpdatecallbacks never fire and the queued-update pump never drains. The editor stalls
rather than just logging an error.
unregisterListenerhas the sameif (unregister) unregister()shape, so the teardown function throws too.Restore the scope the feature was introduced with (#8219, titled "LexicalEditor
RootListener and EditableListener can return unregister callbacks"): keep a
returned callback only for the two maps that document it, and only when it
really is a function.
Test plan
Three new cases in
packages/lexical/src/__tests__/unit/LexicalEditorListener.test.ts, next to theexisting root/editable cleanup-return tests, which still pass unchanged.
Before
Verified by restoring
LexicalUpdates.tsto its pre-fix contents with the newtests in place:
After