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

etrepum commented 6 months ago

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[bot] commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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

etrepum commented 5 months ago

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

acywatson commented 5 months ago

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 commented 5 months ago

This is one of the things that was necessary in this PR: https://github.com/facebook/lexical/pull/3931

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

etrepum commented 5 months ago

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 commented 5 months ago

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!

etrepum commented 5 months ago

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

acywatson commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

etrepum commented 5 months ago

@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 https://github.com/facebook/lexical/pull/5223/commits/2c5b2108dfd1dca6aa78e1d36f2dede2d5e5b6f4

acywatson commented 5 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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!

facebook / lexical

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