Improve TypeScript types by removing `[k: string]: any` from LexicalNode #5223

etrepum · 2023-11-10T18:54:56Z

This refactoring improves the ability for TypeScript to catch errors, particularly typos, because otherwise any unknown method would pass the type checker. I did find one bug in LinkNode (#5221) and some tests that were using textNode.length instead of textNode.__text.length.

The most awkward part was adding an explicit type to the constructors for the classes to implement Klass more precisely. I think that subclasses shouldn't typically need to do this since the superclass' type will generally be sufficient.

Another perhaps controversial addition was adding branded types for isNestedListNode and $isRootOrShadowRoot so that a more precise type could be refined with a predicate in a way that doesn't make TypeScript try and infer the wrong thing (e.g. if f(x): x is Node returns false then it will also infer !(x is Node) but that is not desired when we are looking for something more precise than the type alone.

vercel · 2023-11-10T18:55:01Z

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	Dec 7, 2023 7:30am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 7, 2023 7:30am

etrepum · 2023-11-12T00:38:10Z

I think the failing collab tests are just flaky, based on trying them locally. The failing mac tests were a network error.

npm run test-e2e-collab-prod-chromium List.spec.mjs:1422

> @lexical/monorepo@0.12.2 test-e2e-collab-prod-chromium
> cross-env E2E_BROWSER=chromium E2E_PORT=4000 E2E_EDITOR_MODE=rich-text-with-collab playwright test --project="chromium" List.spec.mjs:1422


Running 1 test using 1 worker

  ✘  1 …/__tests__/e2e/List.spec.mjs:1422:3 › Nested List › remove list breaks when selection in empty nested list item 2 (4s)
  ✓  2 …e2e/List.spec.mjs:1422:3 › Nested List › remove list breaks when selection in empty nested list item 2 (retry #1) (2s)

etrepum · 2023-11-12T19:52:22Z

Looks like another flaky collab test, I can reproduce the flakiness locally. Seems to fail more often than not with FIrefox but I haven't reproduced a failure with chromium.

❯ E2E_PORT=4000 npm run test-e2e-collab-firefox Toolbar.spec.mjs:37

> @lexical/monorepo@0.12.2 test-e2e-collab-firefox
> cross-env E2E_BROWSER=firefox E2E_EDITOR_MODE=rich-text-with-collab playwright test --project="firefox" Toolbar.spec.mjs:37


Running 1 test using 1 worker

  ✘  1 …refox] › packages/lexical-playground/__tests__/e2e/Toolbar.spec.mjs:37:3 › Toolbar › Insert image caption + table (5s)
  ✓  2 …ckages/lexical-playground/__tests__/e2e/Toolbar.spec.mjs:37:3 › Toolbar › Insert image caption + table (retry #1) (3s)


  1) [firefox] › packages/lexical-playground/__tests__/e2e/Toolbar.spec.mjs:37:3 › Toolbar › Insert image caption + table 

    Error: expect(received).toEqual(expected) // deep equality

    - Expected  - 0
    + Received  + 1

    @@ -13,10 +13,11 @@
              spellcheck="true"
              data-lexical-editor="true">
              <p dir="ltr">
                <span data-lexical-text="true">Yellow flower in tilt shift lens</span>
              </p>
    +         <p><br /></p>
            </div>
          </div>
        </span>
        <br />
      </p>

       at packages/lexical-playground/__tests__/utils/index.mjs:159

      157 |     ignoreInlineStyles,
      158 |   });
    > 159 |   expect(actual).toEqual(expected);
          |                  ^
      160 | }
      161 |
      162 | export async function assertHTML(

        at assertHTMLOnPageOrFrame (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:159:18)
        at retryAsync (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:204:7)
        at withRetry (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:169:37)
        at async Promise.all (index 0)
        at assertHTML (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:170:5)
        at file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/e2e/Toolbar.spec.mjs:47:5


  1 flaky
    [firefox] › packages/lexical-playground/__tests__/e2e/Toolbar.spec.mjs:37:3 › Toolbar › Insert image caption + table

etrepum · 2023-11-13T17:50:02Z

More flaky tests in isCollab, here's a few of them

❯ E2E_PORT=4000 npm run test-e2e-collab-chromium packages/lexical-playground/__tests__/e2e/TextEntry.spec.mjs:121

> @lexical/monorepo@0.12.2 test-e2e-collab-chromium
> cross-env E2E_BROWSER=chromium E2E_EDITOR_MODE=rich-text-with-collab playwright test --project="chromium" packages/lexical-playground/__tests__/e2e/TextEntry.spec.mjs:121


Running 1 test using 1 worker

  ✘  1 …cal-playground/__tests__/e2e/TextEntry.spec.mjs:121:3 › TextEntry › Can insert a paragraph between two text nodes (4s)
  ✓  2 …und/__tests__/e2e/TextEntry.spec.mjs:121:3 › TextEntry › Can insert a paragraph between two text nodes (retry #1) (2s)


  1) [chromium] › packages/lexical-playground/__tests__/e2e/TextEntry.spec.mjs:121:3 › TextEntry › Can insert a paragraph between two text nodes 

    Error: expect(received).toEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 5

    - <p dir="ltr"><span data-lexical-text="true">Hello</span></p>
    + <p dir="ltr">
    +   <span data-lexical-text="true">Hello</span>
    +   <strong data-lexical-text="true">w</strong>
    + </p>
    - <p dir="ltr"><strong data-lexical-text="true">world</strong></p>
    + <p dir="ltr"><strong data-lexical-text="true">orld</strong></p>

       at packages/lexical-playground/__tests__/utils/index.mjs:159

      157 |     ignoreInlineStyles,
      158 |   });
    > 159 |   expect(actual).toEqual(expected);
          |                  ^
      160 | }
      161 |
      162 | export async function assertHTML(

        at assertHTMLOnPageOrFrame (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:159:18)
        at retryAsync (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:204:7)
        at withRetry (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:169:37)
        at async Promise.all (index 0)
        at assertHTML (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:170:5)
        at file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/e2e/TextEntry.spec.mjs:133:5


  1 flaky
    [chromium] › packages/lexical-playground/__tests__/e2e/TextEntry.spec.mjs:121:3 › TextEntry › Can insert a paragraph between two text nodes

> cross-env E2E_BROWSER=chromium E2E_PORT=4000 E2E_EDITOR_MODE=rich-text-with-collab playwright test --project="chromium" Links.spec.mjs:425


Running 1 test using 1 worker

  ✘  1 …5:3 › Links › Can create a link with some text after, insert paragraph, then backspace, it should merge correctly (4s)
  ✓  2 … › Can create a link with some text after, insert paragraph, then backspace, it should merge correctly (retry #1) (2s)


  1) [chromium] › packages/lexical-playground/__tests__/e2e/Links.spec.mjs:425:3 › Links › Can create a link with some text after, insert paragraph, then backspace, it should merge correctly 

    Error: expect(received).toEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 2

    @@ -3,20 +3,20 @@
        <a
          class="PlaygroundEditorTheme__link PlaygroundEditorTheme__ltr"
          dir="ltr"
          href="https://"
          rel="noreferrer">
    -     <span data-lexical-text="true">ab</span>
    +     <span data-lexical-text="true">a</span>
        </a>
      </p>
      <p
        class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
        dir="ltr">
        <a
          class="PlaygroundEditorTheme__link PlaygroundEditorTheme__ltr"
          dir="ltr"
          href="https://"
          rel="noreferrer">
    -     <span data-lexical-text="true">c</span>
    +     <span data-lexical-text="true">bc</span>
        </a>
        <span data-lexical-text="true">def</span>
      </p>

       at packages/lexical-playground/__tests__/utils/index.mjs:159

      157 |     ignoreInlineStyles,
      158 |   });
    > 159 |   expect(actual).toEqual(expected);
          |                  ^
      160 | }
      161 |
      162 | export async function assertHTML(

        at assertHTMLOnPageOrFrame (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:159:18)
        at retryAsync (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:204:7)
        at withRetry (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:169:37)
        at async Promise.all (index 0)
        at assertHTML (file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/utils/index.mjs:170:5)
        at file:///Users/bob/src/lexical/packages/lexical-playground/__tests__/e2e/Links.spec.mjs:459:5


  1 flaky
    [chromium] › packages/lexical-playground/__tests__/e2e/Links.spec.mjs:425:3 › Links › Can create a link with some text after, insert paragraph, then backspace, it should merge correctly

acywatson · 2023-11-23T03:26:53Z

Thanks for this - would be great to get it merged.

etrepum · 2023-11-23T03:33:19Z

Happy to do whatever I can to speed that along, short of rewriting the collaboration support to be consistent in tests 😅

acywatson · 2023-11-23T03:43:13Z

Happy to do whatever I can to speed that along, short of rewriting the collaboration support to be consistent in tests 😅

yea don't worry about those tests. I skimmed this and I don't see an issue, but since it's a larger change let me just make sure the rest of the core team has a chance to object. I'll ping internally after the holidays.

GermanJablo · 2023-11-24T04:36:52Z

This is one of the things that was necessary in this PR: #3931

However, it seems better that this PR addresses it in isolation. Very cool!

etrepum · 2023-12-06T19:49:08Z

Let me know if there's anything I can do to move this along. I'm willing to do the work of rebasing and resolving conflicts once or twice if that gets it into a release.

acywatson · 2023-12-06T20:02:08Z

Let me know if there's anything I can do to move this along. I'm willing to do the work of rebasing and resolving conflicts once or twice if that gets it into a release.

I actually just pinged internally about this and I don't think there are any objections here. If you don't mind rebasing, I will approve and merge. Thanks!

This refactoring improves the ability for TypeScript to catch errors, particularly typos, because otherwise any unknown method would pass the type checker. I did find one bug in LinkNode (facebook#5221) and some tests that were using textNode.length instead of textNode.__text.length. The most awkward part was adding an explicit type to the constructors for the classes to implement Klass<T> more precisely. I think that subclasses shouldn't typically need to do this since the superclass' type will generally be sufficient. Another perhaps controversial addition was adding branded types for isNestedListNode and $isRootOrShadowRoot so that a more precise type could be refined with a predicate in a way that doesn't make TypeScript try and infer the wrong thing (e.g. if `f(x): x is Node` returns false then it will also infer `!(x is Node)` but that is not desired when we are looking for something more precise than the type alone.

etrepum · 2023-12-06T23:43:43Z

Should be good to go now, if there's any feedback or unexpected test failures I'm happy to address them

acywatson · 2023-12-07T03:51:11Z

Should be good to go now, if there's any feedback or unexpected test failures I'm happy to address them

Thanks!

This one is actually a little suspicious, because it's failing consistently across collab suites:

[1] 1 failed
[1] [chromium] › packages/lexical-playground/tests/e2e/CopyAndPaste/html/ImageHTMLCopyAndPaste.spec.mjs:64:3 › HTML Image CopyAndPaste › Copy + paste + undo multiple image

Typically with flakiness on collab we'll see most suites pass, or a couple fail with different tests. Usually the same test failing across multiple suites (and runs, since I re-ran) is indicative of a problem somewhere.

Not sure if you have any ideas of what it could be off the top of your head.

etrepum · 2023-12-07T06:56:44Z

I don't think that is a regression in this PR, I can reproduce a consistent failure of that test locally from main and several older tags. I couldn't find any tag where this test passed. I did not do an exhaustive search, just a few in the v0.12.x and v0.11.x series by hand.

etrepum · 2023-12-07T07:10:12Z

Scratch that, I was running the tests wrong (forgot to start the collab server) and can repro a success now. Will look closer!

etrepum · 2023-12-07T07:30:59Z

@acywatson so it turns out that this test is timing dependent, doesn't have its own sleep in the right place, but it worked because it was dependent on a distant bug which caused an unintentional unconditional sleep. I had fixed that bug because the type was wrong (parentheses in the wrong place for an await expression). I restored the unconditional sleep and now it works again. See 2c5b210

acywatson · 2023-12-07T15:12:31Z

@acywatson so it turns out that this test is timing dependent, doesn't have its own sleep in the right place, but it worked because it was dependent on a distant bug which caused an unintentional unconditional sleep. I had fixed that bug because the type was wrong (parentheses in the wrong place for an await expression). I restored the unconditional sleep and now it works again. See 2c5b210

Beautiful! Thank you. We really need to sit down and have a look at the test setup holistically.

alicercedigital · 2024-02-15T00:05:53Z

This change causes the append method to not be recognized as a type.

editor.registerCommand(
    ADD_TEXT_WITH_CHUNKS,
    (payload) => {
     const root = $getRoot();
     root.getFirstChild()?.append($createTextNode(payload));
     return false;
    },
    priority,
   ),
);

Does this mean we are doing this the wrong way?

etrepum · 2024-02-15T00:55:06Z

This change causes the append method to not be recognized as a type.
editor.registerCommand(

    ADD_TEXT_WITH_CHUNKS,

    (payload) => {

     const root = $getRoot();

     root.getFirstChild()?.append($createTextNode(payload));

     return false;

    },

    priority,

   ),

);
Does this mean we are doing this the wrong way?

Yeah, there's no type level guarantee that getFirstChild is an ElementNode

etrepum · 2024-02-15T02:46:32Z

@alicercedigital There are a couple ways to do this that TypeScript would not complain about. The reason it worked before is because node.anyTypoYouCouldImagine would be typed as any, so it would not stop you from calling any method at all, even ones that did not exist, so long as it didn't have an existing type that conflicted with what you were doing. According to the type checker, here you are trying to call append() on a LexicalNode, which should be an error because it is only implemented by ElementNode.

For whatever reason, there's an unsound generic cast available on getFirstChild (which predates this PR), so you can write:

root.getFirstChild<ElementNode>()?.append($createTextNode(payload));

This is unsound in general, and exactly what you were doing before at runtime, but RootNode's children should only be ElementNode so it's not an issue in practice unless there are other bugs.

If you wanted to use the same logical code, but with a runtime type check, you would need to make this two lines:

const parent = root.getFirstChild();
if ($isElementNode(parent)) {
  parent.append($createTextNode(payload));
}

You could also use LexicalNode's getTopLevelElement, which is basically the same as 2 but does a little more work since it looks up the root again before the type check:

root.getFirstChild()?.getTopLevelElement()?.append($createTextNode(payload));

alicercedigital · 2024-02-15T18:20:27Z

@alicercedigital There are a couple ways to do this that TypeScript would not complain about. The reason it worked before is because node.anyTypoYouCouldImagine would be typed as any, so it would not stop you from calling any method at all, even ones that did not exist, so long as it didn't have an existing type that conflicted with what you were doing. According to the type checker, here you are trying to call append() on a LexicalNode, which should be an error because it is only implemented by ElementNode. [...]

Thank you for the careful and explanatory response!

etrepum requested review from zurfyx, fantactuka, acywatson and thegreatercurve as code owners November 10, 2023 18:54

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 Nov 10, 2023

vercel bot had a problem deploying to Preview – lexical-playground November 10, 2023 18:55 Failure

vercel bot deployed to Preview – lexical November 10, 2023 18:56 View deployment

etrepum force-pushed the improve-LexicalNode-typescript branch from 5e311b1 to 8f59fad Compare November 10, 2023 19:04

vercel bot deployed to Preview – lexical November 10, 2023 19:05 View deployment

vercel bot deployed to Preview – lexical-playground November 10, 2023 19:07 View deployment

vercel bot deployed to Preview – lexical November 12, 2023 00:51 View deployment

vercel bot deployed to Preview – lexical-playground November 12, 2023 00:52 View deployment

vercel bot deployed to Preview – lexical November 12, 2023 19:17 View deployment

vercel bot deployed to Preview – lexical-playground November 12, 2023 19:18 View deployment

etrepum force-pushed the improve-LexicalNode-typescript branch from 8c841df to e913dc1 Compare November 13, 2023 17:50

vercel bot deployed to Preview – lexical November 13, 2023 17:51 View deployment

vercel bot deployed to Preview – lexical-playground November 13, 2023 17:52 View deployment

etrepum force-pushed the improve-LexicalNode-typescript branch 2 times, most recently from 9a569db to c10340d Compare November 13, 2023 18:11

vercel bot deployed to Preview – lexical November 13, 2023 18:13 View deployment

vercel bot deployed to Preview – lexical-playground November 13, 2023 18:15 View deployment

etrepum force-pushed the improve-LexicalNode-typescript branch from c10340d to fa9bb80 Compare November 18, 2023 04:59

vercel bot deployed to Preview – lexical November 18, 2023 05:01 View deployment

vercel bot deployed to Preview – lexical-playground November 18, 2023 05:01 View deployment

etrepum force-pushed the improve-LexicalNode-typescript branch from fa9bb80 to 50c138c Compare December 6, 2023 23:29

vercel bot had a problem deploying to Preview – lexical-playground December 6, 2023 23:30 Failure

vercel bot deployed to Preview – lexical December 6, 2023 23:31 View deployment

etrepum force-pushed the improve-LexicalNode-typescript branch from 50c138c to fad9109 Compare December 6, 2023 23:36

vercel bot deployed to Preview – lexical December 6, 2023 23:37 View deployment

vercel bot deployed to Preview – lexical-playground December 6, 2023 23:39 View deployment

Fix await in focusEditor

2c5b210

etrepum force-pushed the improve-LexicalNode-typescript branch from fad9109 to 2c5b210 Compare December 7, 2023 07:27

vercel bot deployed to Preview – lexical December 7, 2023 07:28 View deployment

vercel bot deployed to Preview – lexical-playground December 7, 2023 07:30 View deployment

acywatson approved these changes Dec 7, 2023

View reviewed changes

acywatson merged commit 1a3c911 into facebook:main Dec 7, 2023
45 checks passed

etrepum deleted the improve-LexicalNode-typescript branch May 14, 2024 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve TypeScript types by removing `[k: string]: any` from LexicalNode #5223

Improve TypeScript types by removing `[k: string]: any` from LexicalNode #5223

etrepum commented Nov 10, 2023

vercel bot commented Nov 10, 2023 •

edited

etrepum commented Nov 12, 2023

etrepum commented Nov 12, 2023

etrepum commented Nov 13, 2023

acywatson commented Nov 23, 2023

etrepum commented Nov 23, 2023

acywatson commented Nov 23, 2023

GermanJablo commented Nov 24, 2023

etrepum commented Dec 6, 2023

acywatson commented Dec 6, 2023

etrepum commented Dec 6, 2023

acywatson commented Dec 7, 2023

etrepum commented Dec 7, 2023

etrepum commented Dec 7, 2023

etrepum commented Dec 7, 2023

acywatson commented Dec 7, 2023 •

edited

alicercedigital commented Feb 15, 2024 •

edited

etrepum commented Feb 15, 2024

etrepum commented Feb 15, 2024

alicercedigital commented Feb 15, 2024 •

edited

Improve TypeScript types by removing [k: string]: any from LexicalNode #5223

Improve TypeScript types by removing [k: string]: any from LexicalNode #5223

Conversation

etrepum commented Nov 10, 2023

vercel bot commented Nov 10, 2023 • edited

etrepum commented Nov 12, 2023

etrepum commented Nov 12, 2023

etrepum commented Nov 13, 2023

acywatson commented Nov 23, 2023

etrepum commented Nov 23, 2023

acywatson commented Nov 23, 2023

GermanJablo commented Nov 24, 2023

etrepum commented Dec 6, 2023

acywatson commented Dec 6, 2023

etrepum commented Dec 6, 2023

acywatson commented Dec 7, 2023

etrepum commented Dec 7, 2023

etrepum commented Dec 7, 2023

etrepum commented Dec 7, 2023

acywatson commented Dec 7, 2023 • edited

alicercedigital commented Feb 15, 2024 • edited

etrepum commented Feb 15, 2024

etrepum commented Feb 15, 2024

alicercedigital commented Feb 15, 2024 • edited

Improve TypeScript types by removing `[k: string]: any` from LexicalNode #5223

Improve TypeScript types by removing `[k: string]: any` from LexicalNode #5223

vercel bot commented Nov 10, 2023 •

edited

acywatson commented Dec 7, 2023 •

edited

alicercedigital commented Feb 15, 2024 •

edited

alicercedigital commented Feb 15, 2024 •

edited