Feature: Ensure the type passed to createCommand is retained, inferred, and enforced

[PAkerstrand]

Issue created from a Q-and-A post over at Discord: https://discord.com/channels/953974421008293909/955972012541628456/990966752110325771

I really dig the command system in Lexical, but there are two things that irks me a little bit:

1. Strongly typing the command seems like a great idea, but the underlying types makes it a bit hard to debug the code. Basically, createCommand()-call just creates an empty object {} that's used as a unique key for a Map. This is great, but given that approach, whenever a command is dispatched, the type is simply {}. Would it make sense to have some internal field or name to trace which command is being dispatched?

2. Each LexicalCommand is generic in terms of the payload, so you can have a LexicalCommand<boolean>, or LexicalCommand<KeyboardEvent>. However, the LexicalCommand<T> is really just an alias for Readonly<Record<string, unknown>>, so the required type of the payload isn't saved. This also makes us lose type safety down the line, where you can do

const SOME_EVENT = createCommand<boolean>();

// ...

editor.dispatchCommand(SOME_EVENT, 'Not a boolean...');

and TypeScript will happily accept it. It'd be good if the payload was constrained by the type of the command here.

Additionally, when you registerCommand you need to repeat the type of the payload. Given that the type information was stored inside of the command itself, this could then be inferred, along the lines of:

editor.registerCommand(SOME_EVENT, (payload) => { /* do stuff with inferred boolean */ }

Finally, if the registerCommand could infer the payload type from the command itself, then we could ensure that the command handler has the expected signature. With the current approach there is nothing preventing a very similar error as in the dispatchCommand example, where the registerCommand-handler expects a payload differing from the one provided to createCommand.

---

As a starting point to the second bullet, I've created a PoC draft PR. There are still some type checking errors introduced by this change, related to LexicalEvents:

packages/lexical/src/LexicalEvents.ts:514:68 - error TS2345: Argument of type 'InputEvent' is not assignable
  to parameter of type 'string'.

514         dispatchCommand(editor, CONTROLLED_TEXT_INSERTION_COMMAND, event);
                                                                       ~~~~~

packages/lexical/src/LexicalEvents.ts:521:68 - error TS2345: Argument of type 'InputEvent' is not assignable
  to parameter of type 'string'.

521         dispatchCommand(editor, CONTROLLED_TEXT_INSERTION_COMMAND, event);
                                                                       ~~~~~

packages/lexical/src/LexicalEvents.ts:550:48 - error TS2345: Argument of type 'InputEvent' is not assignable
  to parameter of type 'ClipboardEvent'.
  Property 'clipboardData' is missing in type 'InputEvent' but required in type 'ClipboardEvent'.

550         dispatchCommand(editor, PASTE_COMMAND, event);
                                                   ~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:3533:14
    3533     readonly clipboardData: DataTransfer | null;
                      ~~~~~~~~~~~~~
    'clipboardData' is declared here.

packages/lexical/src/LexicalEvents.ts:872:47 - error TS2345: Argument of type 'KeyboardEvent' is not 
  assignable to parameter of type 'ClipboardEvent'.
  Property 'clipboardData' is missing in type 'KeyboardEvent' but required in type 'ClipboardEvent'.

872         dispatchCommand(editor, COPY_COMMAND, event);
                                                  ~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:3533:14
    3533     readonly clipboardData: DataTransfer | null;
                      ~~~~~~~~~~~~~
    'clipboardData' is declared here.

packages/lexical/src/LexicalEvents.ts:875:46 - error TS2345: Argument of type 'KeyboardEvent' is not
  assignable to parameter of type 'ClipboardEvent'.

875         dispatchCommand(editor, CUT_COMMAND, event);
                                                 ~~~~~

[thegreatercurve]

Thanks for these @PAkerstrand. Your working solution to 2 seems really strong. We used to have payload to command type inference when we used Flow, but we must have lost that when we auto-generated the new TypeScript types.

If you have type issues by changing any of these to union type in any of the registerCommand callbacks, use instanceof in the registerCommand if we shouldn't be calling any subsequent functions i.e.:

 editor.registerCommand(
      CUT_COMMAND,
      (event) => {
         const selection = $getSelection();
        if (!$isRangeSelection(selection)) {
          return false;
        }
        if (event instanceof ClipboardEvent) {
          onCutForPlainText(event, editor);        
        }
        return true;
      },
      COMMAND_PRIORITY_EDITOR,
    ),

The other issue you identified in 1. is definitely on my radar as it will affect our ability to add command logging as part of #2393. Sadly, I'm actually not sure if there's a way around it without changing the existing API. We don't want to require the developer to have to pass a type argument, but we also can't access the instantiating variable name in the enclosing scope of createCommand.

Still not sure of a solution, but I will keep thinking.

[PAkerstrand]

Thanks for the feedback @thegreatercurve 👌🏼

Yeah, I agree that the solution to nr. 1 isn't really clear. I guess it would be possible to use some source transformations to add a string property to the command itself, based on the value of the variable the command is stored in. That would, however, require a transformation step.

Another solution would be to not use an object, but instead a function.. Because they have Function.name built in. So instead of creating an {}. However, that doesn't really work with the createCommand()-pattern either, since the "generated" function would get '' as .name.

---

Anyway, I'll polish up the PoC PR a bit, and let's take it from there

[PAkerstrand]

@thegreatercurve I'm having some issues with various events not being available in the unit tests. I guess this is down to the testing environment in Jest and what's exposed by the jsdom-environment?

 LexicalSelection tests › Paste text, move selection and delete word forward (#129)

    ReferenceError: ClipboardEvent is not defined

      381 |     const selection = $getSelection();
      382 |     const clipboardData =
    > 383 |       event instanceof ClipboardEvent ? event.clipboardData : null;
          |                        ^
      384 |     if (
      385 |       clipboardData != null &&
      386 |       ($isRangeSelection(selection) || $isGridSelection(selection))

      at packages/lexical-rich-text/src/index.ts:383:24
      at processNestedUpdates (packages/lexical/src/LexicalUpdates.ts:738:7)
      at beginUpdate (packages/lexical/src/LexicalUpdates.ts:796:22)
      at updateEditor (packages/lexical/src/LexicalUpdates.ts:902:5)
      at LexicalEditor.update (packages/lexical/src/LexicalEditor.ts:746:17)
      at packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx:159:20
      at Object.act (node_modules/react/cjs/react.development.js:2510:16)
      at update (packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx:158:26)
      at applySelectionInputs (packages/lexical-selection/src/__tests__/utils/index.ts:646:13)
      at Object.<anonymous> (packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx:949:33)

Any ideas whether it is fixable and how to set it up?

[thegreatercurve]

@thegreatercurve I'm having some issues with various events not being available in the unit tests. I guess this is down to the testing environment in Jest and what's exposed by the jsdom-environment?

 LexicalSelection tests › Paste text, move selection and delete word forward (#129)

    ReferenceError: ClipboardEvent is not defined

      381 |     const selection = $getSelection();
      382 |     const clipboardData =
    > 383 |       event instanceof ClipboardEvent ? event.clipboardData : null;
          |                        ^
      384 |     if (
      385 |       clipboardData != null &&
      386 |       ($isRangeSelection(selection) || $isGridSelection(selection))

      at packages/lexical-rich-text/src/index.ts:383:24
      at processNestedUpdates (packages/lexical/src/LexicalUpdates.ts:738:7)
      at beginUpdate (packages/lexical/src/LexicalUpdates.ts:796:22)
      at updateEditor (packages/lexical/src/LexicalUpdates.ts:902:5)
      at LexicalEditor.update (packages/lexical/src/LexicalEditor.ts:746:17)
      at packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx:159:20
      at Object.act (node_modules/react/cjs/react.development.js:2510:16)
      at update (packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx:158:26)
      at applySelectionInputs (packages/lexical-selection/src/__tests__/utils/index.ts:646:13)
      at Object.<anonymous> (packages/lexical-selection/src/__tests__/unit/LexicalSelection.test.tsx:949:33)

Any ideas whether it is fixable and how to set it up?

@PAkerstrand Interesting.

I think this might be a bug within Jest or JSDOM. From a quick experiment, it works the other way around if you do the check as event instanceof KeyboardEvent ? null : event.clipboardData;.

That should be enough to overcome the CI issues.

[thegreatercurve]

Closed by #2537