[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection #6420

etrepum · 2024-07-18T19:56:10Z

Description

Targeted fix for lexical-history's TextNode change detection when only one text leaf is dirty but unchanged
Add delay option to HistoryPlugin
Fix $cloneWithProperties - the previous implementation did not correctly handle __textFormat from ParagraphNode. Also updated the very incorrect doc string.
Clean up lint and test warning issues elsewhere
Move $cloneWithProperties to the lexical package because the correct implementation was already inlined in LexicalNode.getWritable()

A more general fix may be possible, it's unclear whether this hack is really necessary for MaxLengthPlugin but other text node transforms may have similar issues so removing the hack could be backwards incompatible.

Closes #6409

Test plan

Before

History's TextNode change detection did not work correctly for subclasses where only properties that did not exist on TextNode changed.

After

The tests show that the behavior is consistent with expectations

vercel · 2024-07-18T19:56:17Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 19, 2024 2:35pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 19, 2024 2:35pm

github-actions · 2024-07-18T19:58:24Z

size-limit report 📦

Path	Size
lexical - cjs	28.76 KB (0%)
lexical - esm	28.61 KB (0%)
@lexical/rich-text - cjs	37.2 KB (0%)
@lexical/rich-text - esm	28.21 KB (0%)
@lexical/plain-text - cjs	35.85 KB (0%)
@lexical/plain-text - esm	25.5 KB (0%)
@lexical/react - cjs	39.12 KB (0%)
@lexical/react - esm	29.49 KB (0%)

Add delay option to HistoryPlugin Add test for facebook#6409 Fix lint issues Fix $cloneWithProperties and test expectations Targeted fix for TextNode leaf change

…ed in LexicalNode.getWritable())

etrepum · 2024-07-18T22:59:58Z

packages/lexical-selection/src/lexical-node.ts

-  if ($isParagraphNode(node) && $isParagraphNode(clone)) {
-    return $updateParagraphNodeProperties(clone, node);
-  }


This was wrong because ParagraphNode is always an ElementNode and the exit already happened. This was implemented correctly in LexicalNode.getWritable() so I extracted and exported that implementation instead

etrepum · 2024-07-18T23:00:58Z

packages/lexical-utils/src/index.ts

-    if ($isTextNode(clone)) {
-      invariant($isTextNode(node), 'Expected node be a TextNode');
-      clone.__text = node.__text;
-    }


I could not find any possible reason for this code to be here, TextNode.clone already guarantees clone.__text === node.__text

etrepum · 2024-07-18T23:01:12Z

packages/lexical-website/src/components/Gallery/utils.tsx

@@ -51,6 +51,6 @@ export function useFilteredExamples(examples: Array<Example>) {
        searchName,
        tags,
      }),
-    [examples, searchName],
+    [examples, searchName, tags],


etrepum · 2024-07-18T23:01:50Z

packages/lexical/src/LexicalNode.ts

    cloneNotNeeded.add(key);
-    mutableNode.__key = key;


This was not necessary because Node.clone must already do this, I added an invariant to guarantee it

etrepum · 2024-07-18T23:10:02Z

packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx

@@ -2272,42 +2272,44 @@ describe('LexicalSelection tests', () => {
    it('adjust offset for inline elements text formatting', async () => {
      await init();

-      await editor!.update(() => {
-        const root = $getRoot();
+      await ReactTestUtils.act(async () => {


Wrapping this is necessary because the update causes a react update

etrepum · 2024-07-18T23:10:35Z

packages/lexical-react/src/LexicalHistoryPlugin.ts

@@ -17,13 +17,15 @@ export {createEmptyHistoryState} from '@lexical/history';
 export type {HistoryState};

 export function HistoryPlugin({
+  delay,


Seemed reasonable to fix this missing option while I was in here

etrepum · 2024-07-18T23:11:29Z

packages/lexical-react/src/LexicalContentEditable.tsx

@@ -15,7 +15,6 @@ import {forwardRef, Ref, useLayoutEffect, useState} from 'react';
 import {ContentEditableElement} from './shared/LexicalContentEditableElement';
 import {useCanShowPlaceholder} from './shared/useCanShowPlaceholder';

-/* eslint-disable @typescript-eslint/ban-types */


Could not find a reason for this, if there's an internal meta reason we should sync the eslint config to match whatever that rule is

etrepum · 2024-07-18T23:15:34Z

I don't see the typical Lexical Tests checks running on this PR? /cc @potatowagon @Sahejkm

etrepum · 2024-07-19T04:56:34Z

packages/lexical/src/LexicalUtils.ts

+    mutableNode.__mode = latestNode.__mode;
+    mutableNode.__detail = latestNode.__detail;
+  }
+  if (__DEV__) {


I moved this to a __DEV__ because it's a new invariant in a relatively hot code path but we might want to run it unconditionally?

potatowagon · 2024-07-19T07:43:52Z

I don't see the typical Lexical Tests checks running on this PR? /cc @potatowagon @Sahejkm

thats strange, why are the jobs skipped, will investigate

potatowagon · 2024-07-19T07:51:32Z

it seems like the core test runs are missing

potatowagon · 2024-07-19T07:55:39Z

it seems like the core test runs are missing

think its because of this:

lexical/.github/workflows/tests.yml

Line 12 in 666ccb2

- 'packages/lexical-website/**'

could be its ignoring core test trigger because this PR modifies the lexical website path. will fix it

potatowagon · 2024-07-19T08:12:23Z

it seems like the core test runs are missing

think its because of this:

lexical/.github/workflows/tests.yml

Line 12 in 666ccb2

- 'packages/lexical-website/**'

could be its ignoring core test trigger because this PR modifies the lexical website path. will fix it

fixed in #6423

potatowagon · 2024-07-19T08:40:56Z

it seems like the core test runs are missing

think its because of this:

lexical/.github/workflows/tests.yml

Line 12 in 666ccb2

- 'packages/lexical-website/**'

could be its ignoring core test trigger because this PR modifies the lexical website path. will fix it

fixed in #6423

prolly not the reason since core tests are triggered on #6425. im not sure what else is causing the core test jobs to not trigger on this PR. could it be a transient issue? at least on test PR #6425 the core tests are triggering

potatowagon

thankyou for the fix! changes look fine according to e2e test results

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 18, 2024

vercel bot deployed to Preview – lexical July 18, 2024 20:00 View deployment

vercel bot deployed to Preview – lexical-playground July 18, 2024 20:00 View deployment

etrepum added 3 commits July 18, 2024 15:56

Fix facebook#6409 TextNode change detection

79528d1

Add delay option to HistoryPlugin Add test for facebook#6409 Fix lint issues Fix $cloneWithProperties and test expectations Targeted fix for TextNode leaf change

Move $cloneWithProperties to lexical packages (code was already inlin…

1b53497

…ed in LexicalNode.getWritable())

Fix warning: Placeholder inside a test was not wrapped in act(...)

1e0187a

etrepum force-pushed the isTextNodeUnchanged branch from d7fca44 to 1e0187a Compare July 18, 2024 22:56

vercel bot deployed to Preview – lexical July 18, 2024 22:58 View deployment

vercel bot deployed to Preview – lexical-playground July 18, 2024 22:58 View deployment

etrepum commented Jul 18, 2024

View reviewed changes

move invariant to __DEV__ since this is a hot code path

089049a

etrepum commented Jul 18, 2024

View reviewed changes

vercel bot deployed to Preview – lexical July 18, 2024 23:10 View deployment

vercel bot deployed to Preview – lexical-playground July 18, 2024 23:10 View deployment

etrepum commented Jul 18, 2024

View reviewed changes

etrepum marked this pull request as ready for review July 18, 2024 23:12

etrepum requested review from zurfyx, fantactuka, acywatson, Fetz, ivailop7, Sahejkm and potatowagon as code owners July 18, 2024 23:12

etrepum commented Jul 19, 2024

View reviewed changes

potatowagon added the extended-tests Run extended e2e tests on a PR label Jul 19, 2024

potatowagon mentioned this pull request Jul 19, 2024

CI: remove paths-ignore #6423

Closed

Merge branch 'main' into isTextNodeUnchanged

e8bcd7c

vercel bot deployed to Preview – lexical July 19, 2024 14:35 View deployment

vercel bot deployed to Preview – lexical-playground July 19, 2024 14:35 View deployment

potatowagon approved these changes Jul 19, 2024

View reviewed changes

ivailop7 approved these changes Jul 20, 2024

View reviewed changes

zurfyx approved these changes Jul 22, 2024

View reviewed changes

zurfyx added this pull request to the merge queue Jul 22, 2024

Merged via the queue into facebook:main with commit 78abd30 Jul 22, 2024
50 of 51 checks passed

korbit-ai bot mentioned this pull request Aug 15, 2024

[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection furwellness/lexical#26

Open

local-dev-korbit-ai-mentor bot mentioned this pull request Aug 15, 2024

[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection furwellness/lexical#42

Open

phazingazrael mentioned this pull request Aug 23, 2024

[Snyk] Upgrade @lexical/react from 0.15.0 to 0.17.0 phazingazrael/Terra-Logger#34

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection #6420

[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection #6420

etrepum commented Jul 18, 2024 •

edited

Loading

vercel bot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

etrepum Jul 18, 2024

etrepum Jul 18, 2024

etrepum Jul 18, 2024

etrepum Jul 18, 2024

etrepum Jul 18, 2024

etrepum Jul 18, 2024

etrepum Jul 18, 2024

etrepum commented Jul 18, 2024

etrepum Jul 19, 2024

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024 •

edited

Loading

potatowagon left a comment •

edited

Loading

[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection #6420

[lexical-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection #6420

Conversation

etrepum commented Jul 18, 2024 • edited Loading

Description

Test plan

Before

After

vercel bot commented Jul 18, 2024 • edited Loading

github-actions bot commented Jul 18, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etrepum commented Jul 18, 2024

Choose a reason for hiding this comment

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024

potatowagon commented Jul 19, 2024 • edited Loading

potatowagon left a comment • edited Loading

Choose a reason for hiding this comment

etrepum commented Jul 18, 2024 •

edited

Loading

vercel bot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

potatowagon commented Jul 19, 2024 •

edited

Loading

potatowagon left a comment •

edited

Loading