-
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 1 commit
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
…ds compatibility
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,25 +110,29 @@ function CodeActionMenuContainer({ | |
}, [shouldListenMouseMove, debouncedOnMouseMove]); | ||
|
||
useEffect(() => { | ||
return editor.registerMutationListener(CodeNode, (mutations) => { | ||
editor.getEditorState().read(() => { | ||
for (const [key, type] of mutations) { | ||
switch (type) { | ||
case 'created': | ||
codeSetRef.current.add(key); | ||
break; | ||
|
||
case 'destroyed': | ||
codeSetRef.current.delete(key); | ||
break; | ||
|
||
default: | ||
break; | ||
return editor.registerMutationListener( | ||
CodeNode, | ||
(mutations) => { | ||
editor.getEditorState().read(() => { | ||
for (const [key, type] of mutations) { | ||
switch (type) { | ||
case 'created': | ||
codeSetRef.current.add(key); | ||
break; | ||
|
||
case 'destroyed': | ||
codeSetRef.current.delete(key); | ||
break; | ||
|
||
default: | ||
break; | ||
} | ||
} | ||
} | ||
}); | ||
setShouldListenMouseMove(codeSetRef.current.size > 0); | ||
}); | ||
}); | ||
setShouldListenMouseMove(codeSetRef.current.size > 0); | ||
}, | ||
{skipInitialization: false}, | ||
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. CodeActionMenuPlugin did not previously handle initialization correctly |
||
); | ||
}, [editor]); | ||
|
||
const normalizedLang = normalizeCodeLang(lang); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,17 +183,21 @@ function TableActionMenu({ | |
); | ||
|
||
useEffect(() => { | ||
return editor.registerMutationListener(TableCellNode, (nodeMutations) => { | ||
const nodeUpdated = | ||
nodeMutations.get(tableCellNode.getKey()) === 'updated'; | ||
|
||
if (nodeUpdated) { | ||
editor.getEditorState().read(() => { | ||
updateTableCellNode(tableCellNode.getLatest()); | ||
}); | ||
setBackgroundColor(currentCellBackgroundColor(editor) || ''); | ||
} | ||
}); | ||
return editor.registerMutationListener( | ||
TableCellNode, | ||
(nodeMutations) => { | ||
const nodeUpdated = | ||
nodeMutations.get(tableCellNode.getKey()) === 'updated'; | ||
|
||
if (nodeUpdated) { | ||
editor.getEditorState().read(() => { | ||
updateTableCellNode(tableCellNode.getLatest()); | ||
}); | ||
setBackgroundColor(currentCellBackgroundColor(editor) || ''); | ||
} | ||
}, | ||
{skipInitialization: true}, | ||
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. TableActionMenuPlugin did handle initialization correctly (it only listens for updates) |
||
); | ||
}, [editor, tableCellNode]); | ||
|
||
useEffect(() => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,24 +105,23 @@ export function LexicalAutoEmbedPlugin<TEmbedConfig extends EmbedConfig>({ | |
}, []); | ||
|
||
const checkIfLinkNodeIsEmbeddable = useCallback( | ||
(key: NodeKey) => { | ||
editor.getEditorState().read(async function () { | ||
async (key: NodeKey) => { | ||
const url = editor.getEditorState().read(function () { | ||
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 code was very sketchy previously, and only worked at all because it used |
||
const linkNode = $getNodeByKey(key); | ||
if ($isLinkNode(linkNode)) { | ||
for (let i = 0; i < embedConfigs.length; i++) { | ||
const embedConfig = embedConfigs[i]; | ||
|
||
const urlMatch = await Promise.resolve( | ||
embedConfig.parseUrl(linkNode.__url), | ||
); | ||
|
||
if (urlMatch != null) { | ||
setActiveEmbedConfig(embedConfig); | ||
setNodeKey(linkNode.getKey()); | ||
} | ||
} | ||
return linkNode.getURL(); | ||
} | ||
}); | ||
if (url === undefined) { | ||
return; | ||
} | ||
for (const embedConfig of embedConfigs) { | ||
const urlMatch = await Promise.resolve(embedConfig.parseUrl(url)); | ||
if (urlMatch != null) { | ||
setActiveEmbedConfig(embedConfig); | ||
setNodeKey(key); | ||
} | ||
} | ||
}, | ||
[editor, embedConfigs], | ||
); | ||
|
@@ -146,7 +145,9 @@ export function LexicalAutoEmbedPlugin<TEmbedConfig extends EmbedConfig>({ | |
}; | ||
return mergeRegister( | ||
...[LinkNode, AutoLinkNode].map((Klass) => | ||
editor.registerMutationListener(Klass, (...args) => listener(...args)), | ||
editor.registerMutationListener(Klass, (...args) => listener(...args), { | ||
skipInitialization: true, | ||
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 implementation is not compatible with |
||
}), | ||
), | ||
); | ||
}, [checkIfLinkNodeIsEmbeddable, editor, embedConfigs, nodeKey, reset]); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,6 +149,7 @@ export function TablePlugin({ | |
} | ||
} | ||
}, | ||
{skipInitialization: false}, | ||
); | ||
|
||
return () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,11 +215,15 @@ export type NodeMutation = 'created' | 'updated' | 'destroyed'; | |
export interface MutationListenerOptions { | ||
/** | ||
* Skip the initial call of the listener with pre-existing DOM nodes. | ||
* Default is false. | ||
* | ||
* The default is currently true for backwards compatibility with <= 0.16.1 | ||
* but this default is expected to change to false in 0.17.0. | ||
*/ | ||
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. |
||
} | ||
|
||
const DEFAULT_SKIP_INITIALIZATION = true; | ||
|
||
export type UpdateListener = (arg0: { | ||
dirtyElements: Map<NodeKey, IntentionallyMarkedAsDirtyElement>; | ||
dirtyLeaves: Set<NodeKey>; | ||
|
@@ -834,9 +838,10 @@ 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. | ||
* If any existing nodes are in the DOM, and skipInitialization is not true, the listener | ||
* will be called immediately with an updateTag of 'registerMutationListener' where all | ||
* nodes have the 'created' NodeMutation. This can be controlled with the skipInitialization option | ||
* (default is currently true for backwards compatibility in 0.16.x but will change to false in 0.17.0). | ||
* | ||
* @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. | ||
|
@@ -853,7 +858,12 @@ export class LexicalEditor { | |
).klass; | ||
const mutations = this._listeners.mutation; | ||
mutations.set(listener, klassToMutate); | ||
if (!(options && options.skipInitialization)) { | ||
const skipInitialization = options && options.skipInitialization; | ||
if ( | ||
!(skipInitialization === undefined | ||
? DEFAULT_SKIP_INITIALIZATION | ||
: skipInitialization) | ||
) { | ||
Comment on lines
+860
to
+865
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. Would be |
||
this.initializeMutationListener(listener, klassToMutate); | ||
} | ||
|
||
|
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.
CodeHighlighter did not previously handle initialization correctly
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.
nice catch