-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[lexical] Feature: registerMutationListener should initialize its existing nodes #6357
Changes from 2 commits
a560f2f
c73a70a
7e2d68b
c27ba0f
7f86a8b
b5d9f54
1bb7e1f
63723fa
a8bfbfb
9fa7bb8
314dfb9
9b19859
3d932c4
4b720a1
82913db
47f9438
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 |
---|---|---|
|
@@ -38,7 +38,6 @@ import { | |
$createParagraphNode, | ||
$getNodeByKey, | ||
$isTextNode, | ||
$nodesOfType, | ||
COMMAND_PRIORITY_EDITOR, | ||
} from 'lexical'; | ||
import {useEffect} from 'react'; | ||
|
@@ -129,17 +128,6 @@ export function TablePlugin({ | |
} | ||
}; | ||
|
||
// Plugins might be loaded _after_ initial content is set, hence existing table nodes | ||
// won't be initialized from mutation[create] listener. Instead doing it here, | ||
editor.getEditorState().read(() => { | ||
const tableNodes = $nodesOfType(TableNode); | ||
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. This was not correct because |
||
for (const tableNode of tableNodes) { | ||
if ($isTableNode(tableNode)) { | ||
initializeTableNode(tableNode); | ||
} | ||
} | ||
}); | ||
|
||
const unregisterMutationListener = editor.registerMutationListener( | ||
TableNode, | ||
(nodeMutations) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,20 +30,20 @@ This can be a simple, efficient way to handle some use cases, since it's not nec | |
|
||
## 2. Directly Attach Handlers | ||
|
||
In some cases, it may be better to attach an event handler directly to the underlying DOM node of each specific node. With this approach, you generally don't need to filter the event target in the handler, which can make it a bit simpler. It will also guarantee that you're handler isn't running for events that you don't care about. This approach is implemented via a [Mutation Listener](https://lexical.dev/docs/concepts/listeners). | ||
In some cases, it may be better to attach an event handler directly to the underlying DOM node of each specific node. With this approach, you generally don't need to filter the event target in the handler, which can make it a bit simpler. It will also guarantee that your handler isn't running for events that you don't care about. This approach is implemented via a [Mutation Listener](https://lexical.dev/docs/concepts/listeners). | ||
|
||
```js | ||
const registeredElements: WeakSet<HTMLElement> = new WeakSet(); | ||
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. Just fixing the correctness and formatting of this relevant example, the state needs to be outside of the listener implementation! |
||
const removeMutationListener = editor.registerMutationListener(nodeType, (mutations) => { | ||
const registeredElements: WeakSet<HTMLElement> = new WeakSet(); | ||
editor.getEditorState().read(() => { | ||
for (const [key, mutation] of mutations) { | ||
const element: null | HTMLElement = editor.getElementByKey(key); | ||
if ( | ||
// Updated might be a move, so that might mean a new DOM element | ||
// is created. In this case, we need to add and event listener too. | ||
(mutation === 'created' || mutation === 'updated') && | ||
element !== null && | ||
!registeredElements.has(element) | ||
// Updated might be a move, so that might mean a new DOM element | ||
// is created. In this case, we need to add and event listener too. | ||
(mutation === 'created' || mutation === 'updated') && | ||
element !== null && | ||
!registeredElements.has(element) | ||
) { | ||
registeredElements.add(element); | ||
element.addEventListener('click', (event: Event) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -211,6 +211,14 @@ export type MutatedNodes = Map<Klass<LexicalNode>, Map<NodeKey, NodeMutation>>; | |
|
||
export type NodeMutation = 'created' | 'updated' | 'destroyed'; | ||
|
||
export interface MutationListenerOptions { | ||
/** | ||
* Skip the initial call of the listener with pre-existing DOM nodes. | ||
* Default is false. | ||
*/ | ||
skipInitialization?: boolean; | ||
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. Is this the best term for users? Initialization describes the work done behind the users, for users it's merely skipping the first one. Do we even need? ⭐️ 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. We need an option because on the call it was discussed that we need the default behavior to be exactly the current behavior. When the default changes, it should be very rare to have to specify this option, which is why it should have a name that implies that the default is false. I don't have a strong preference as to whether it's called "skipInitialization" or "doNotSendCreateEventForExistingDOM" or whatever. |
||
} | ||
|
||
export type UpdateListener = (arg0: { | ||
dirtyElements: Map<NodeKey, IntentionallyMarkedAsDirtyElement>; | ||
dirtyLeaves: Set<NodeKey>; | ||
|
@@ -825,13 +833,19 @@ export class LexicalEditor { | |
* One common use case for this is to attach DOM event listeners to the underlying DOM nodes as Lexical nodes are created. | ||
* {@link LexicalEditor.getElementByKey} can be used for this. | ||
* | ||
* If any existing nodes are in the DOM, the listener will be called immediately with | ||
* an updateTag of 'registerMutationListener' where all nodes have the 'created' NodeMutation. | ||
* This behavior can be disabled with the skipInitialization option. | ||
* | ||
* @param klass - The class of the node that you want to listen to mutations on. | ||
* @param listener - The logic you want to run when the node is mutated. | ||
* @param options - see {@link MutationListenerOptions} | ||
* @returns a teardown function that can be used to cleanup the listener. | ||
*/ | ||
registerMutationListener( | ||
klass: Klass<LexicalNode>, | ||
listener: MutationListener, | ||
options?: MutationListenerOptions, | ||
): () => void { | ||
let registeredNode = this._nodes.get(klass.getType()); | ||
|
||
|
@@ -862,12 +876,37 @@ export class LexicalEditor { | |
|
||
const mutations = this._listeners.mutation; | ||
mutations.set(listener, klassToMutate); | ||
if (!(options && options.skipInitialization)) { | ||
this.initializeMutationListener(listener, klassToMutate); | ||
} | ||
|
||
return () => { | ||
mutations.delete(listener); | ||
}; | ||
} | ||
|
||
/** @internal */ | ||
private initializeMutationListener( | ||
listener: MutationListener, | ||
klass: Klass<LexicalNode>, | ||
): void { | ||
const prevEditorState = this._editorState; | ||
const nodeMutationMap = new Map<string, NodeMutation>(); | ||
const klassType = klass.getType(); | ||
for (const [key, node] of prevEditorState._nodeMap) { | ||
if (node.__type === klassType) { | ||
nodeMutationMap.set(key, 'created'); | ||
} | ||
} | ||
if (nodeMutationMap.size > 0) { | ||
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. For the sake of predictability I wonder if we should always call it regardless. This is sort of inline with my previous comment but less radical, I understand that libraries like Rxjs follow similar patterns. ⭐️ This predicatibility also gives users more flexibility and the option flag becomes more of a convenience rather than a must-have. 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. I don't have a strong preference one way or the other, correct code won't notice a difference. The current zero check is "more" backwards compatible in that the behavior only changes if the mutation is registered after the initial reconciliation, so I think more tests will end up having to change a bit (the ones based on call count rather than meaningful substance). Since we ended up with an option and a deprecation phase I think either is fine. Anyone else have an opinion about this? |
||
listener(nodeMutationMap, { | ||
dirtyLeaves: new Set(), | ||
prevEditorState, | ||
updateTags: new Set(['registerMutationListener']), | ||
}); | ||
} | ||
} | ||
|
||
/** @internal */ | ||
private registerNodeTransformToKlass<T extends LexicalNode>( | ||
klass: Klass<T>, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LexicalTableOfContentsPlugin already had an efficient and correct initialization (though this wouldn't break it)