Capture the expected payload type in commands #2537

PAkerstrand · 2022-06-27T22:39:48Z

Implements inference and enforcement of the payload type for LexicalCommand, as discussed in point 2. in #2536.

vercel · 2022-06-27T22:39:54Z

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

Name	Status	Preview	Updated
lexical	✅ Ready (Inspect)	Visit Preview	Jun 30, 2022 at 10:41AM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	Jun 30, 2022 at 10:41AM (UTC)

packages/lexical-code/src/index.ts

packages/lexical-rich-text/src/index.ts

packages/lexical/src/LexicalCommands.ts

thegreatercurve · 2022-06-29T14:10:53Z

packages/lexical-rich-text/src/index.ts

@@ -388,11 +390,15 @@ function onPasteForRichText(
  });
 }

-function onCopyForRichText(event: ClipboardEvent, editor: LexicalEditor): void {
+function onCopyForRichText(
+  event: CommandPayloadType<typeof COPY_COMMAND>,


Could we just use a simple union type here instead? It might be a little more readable and CommandPayloadType feels slightly too general if we're not using it a lot elsewhere.

Suggested change

event: CommandPayloadType<typeof COPY_COMMAND>,

event: ClipboardEvent | KeyboardEvent,

Since these are actually command listeners, as opposed to DOM Event Listeners, I think the abstraction here is appropriate.

I was going to actually suggest going a step further and having

type CopyCommandPayload = typeof COPY_COMMAND; ... event: CommandPayloadType<CopyCommandPayload>;

@zurfyx @fantactuka @tylerjbainbridge ?

@thegreatercurve I actually started with just a union type. The question then becomes, do we want to provide a utility type for extracting the payload in a nice way from the lexical package, to make it a bit easier for other users if they want to extract a function for delegating to from the command handlers? Inside of the command handlers the payload is inferred, but that's only the case if/when it is inlined. If you want to do something like this:

type MY_COMMAND = createCommand<string | SomeOtherType>(); // ... editor.registerCommand(MY_COMMAND, myCommandHandler); // ... function myCommandHandler(payload: string | SomeOtherType) {}

The idea here was to provide functionality for this, such that the payload type and the argument type are can easily be kept in sync if that union type would grow for some reason

function myCommandHandler(payload: CommandPayloadType<typeof MY_COMMAND>) {}

If we do want to provide it, it would make a lot of sense to adorn it with a proper TSDoc comment to show its purpose and some examples of how to use it.

I guess it's also a question of:

How often do the generic type of a command really change?

Where would we want type errors in usage to pop up?

thegreatercurve · 2022-06-29T14:11:50Z

Closes #2536

thegreatercurve · 2022-06-29T14:12:15Z

Thanks for looking into this @PAkerstrand!

zurfyx

Thanks for taking a stab at this, we certainly missed this when we migrated to TS and it's pretty important to get this right! Turns out TS resolves the type whereas Flow takes the first type available so we can no longer leverage the empty object and unused generic trick

Wrote down a couple comments, the idea is correct but I think the clipboard types needs fixing..
I wrote a simple version outlining the gist of the problem, it comes down to a current issue on the existing codebase I think (lmk if I'm mistaken, I checked pretty quick) 5f743c9

packages/lexical/src/LexicalEditor.ts

packages/lexical-plain-text/src/index.ts

packages/lexical/src/LexicalEditor.ts

PAkerstrand · 2022-06-30T07:08:46Z

packages/lexical/src/LexicalEditor.ts

+export type CommandPayloadType<C> = C extends LexicalCommand<infer R>
+  ? R
+  : never;


I moved this type helper here since it is quite tightly tied to the LexicalCommand-type

packages/lexical/src/LexicalEditor.ts

PAkerstrand · 2022-06-30T07:10:48Z

packages/lexical/src/LexicalEditor.ts

+  dispatchCommand<
+    TCommand extends LexicalCommand<unknown>,
+    TPayload extends CommandPayloadType<TCommand>,
+  >(type: TCommand, payload: TPayload): boolean {


@thegreatercurve Now we have some internal usage of the CommandPayloadType, namely to ensure that the type of the payload matches what's extracted from the type of the first argument

PAkerstrand · 2022-06-30T07:11:38Z

packages/lexical/src/LexicalEvents.ts

@@ -482,7 +482,7 @@ function onBeforeInput(event: InputEvent, editor: LexicalEditor): void {
    if (inputType === 'insertText') {
      if (data === '\n') {
        event.preventDefault();
-        dispatchCommand(editor, INSERT_LINE_BREAK_COMMAND, undefined);
+        dispatchCommand(editor, INSERT_LINE_BREAK_COMMAND, false);


New type errors that popped up from the stricter enforcement.

A LexicalCommand<boolean> no longer accepts undefined or null as payload

PAkerstrand · 2022-06-30T07:12:22Z

packages/lexical/src/LexicalEvents.ts

+                  return dispatchCommand(
+                    editor,
+                    CUT_COMMAND,
+                    event as ClipboardEvent,
+                  );


Are we fine with these type casts, or do we need to refactor the code such that TS can properly infer the type itself?

PAkerstrand · 2022-06-30T07:13:20Z

packages/lexical/src/LexicalUtils.ts

+export function dispatchCommand<
+  TCommand extends LexicalCommand<unknown>,
+  TPayload extends CommandPayloadType<TCommand>,
+>(editor: LexicalEditor, type: TCommand, payload: TPayload): boolean {


Same signature as in LexicalEditor.ts

PAkerstrand · 2022-06-30T08:27:42Z

I keep getting these types of failures in the test suite, and it's usually on Windows and composition. Not sure whether it's coincidence or if I've accidentally introduced some errors? Not sure how I could introduce connection refused errors though.

PAkerstrand · 2022-06-30T10:43:11Z

packages/lexical/src/LexicalEditor.ts

@@ -167,7 +167,30 @@ export const COMMAND_PRIORITY_HIGH = 3;
 export const COMMAND_PRIORITY_CRITICAL = 4;

 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-export type LexicalCommand<T = never> = Readonly<Record<string, unknown>>;
+export type LexicalCommand<TPayload> = Record<string, never>;


Turns out it was the Readonly type helper that breaks type inference later on. Record<string, never> signifies an empty object, which is what the commands currently are, but still allows the generic type to be extracted later on.

PAkerstrand · 2022-06-30T12:13:14Z

I keep getting these types of failures in the test suite, and it's usually on Windows and composition. Not sure whether it's coincidence or if I've accidentally introduced some errors? Not sure how I could introduce connection refused errors though.

This seems to be the core issue of the failed runs, we get an EMFILE-error emitted. Might be some file descriptor leakage somewhere?

acywatson · 2022-07-04T23:26:15Z

@thegreatercurve - the author let me know he is on vacation for the next 3 weeks - can you take this over and see if we need to make any changes or if we can merge it as-is?

The CI failure is the Windows thing I fixed and should pass on rebase (or we can ignore that signal and merge as-is if you're good)

PAkerstrand · 2022-07-05T09:50:27Z

@thegreatercurve - the author let me know he is on vacation for the next 3 weeks - can you take this over and see if we need to make any changes or if we can merge it as-is?

The CI failure is the Windows thing I fixed and should pass on rebase (or we can ignore that signal and merge as-is if you're good)

Correct ☝🏼

I’m completely OK with you pushing any changes required to get this merged. I am somewhat around to be able to respond, but won’t be coding for the next few weeks

Previously typed as `string`, updated to `InputEvent | string`

The `jsdom`-environment in Jest does not have `ClipboardEvent` globally defined, causing the unit tests to fail

Additionally, use generic contraints to enforce the command type and payload type to match. Fix type errors from stricter enforcements introduced

add some TSDoc for `CommandPayloadType`-helper

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 Jun 27, 2022

vercel bot deployed to Preview – lexical June 27, 2022 22:40 View deployment

vercel bot had a problem deploying to Preview – lexical-playground June 27, 2022 22:40 Failure

vercel bot had a problem deploying to Preview – lexical-playground June 27, 2022 22:50 Failure

vercel bot deployed to Preview – lexical June 27, 2022 22:51 View deployment

PAkerstrand commented Jun 27, 2022

View reviewed changes

packages/lexical-code/src/index.ts Show resolved Hide resolved

packages/lexical-rich-text/src/index.ts Show resolved Hide resolved

packages/lexical/src/LexicalCommands.ts Show resolved Hide resolved

vercel bot deployed to Preview – lexical June 28, 2022 14:28 View deployment

vercel bot deployed to Preview – lexical-playground June 28, 2022 14:29 View deployment

vercel bot deployed to Preview – lexical June 29, 2022 08:01 View deployment

vercel bot deployed to Preview – lexical-playground June 29, 2022 08:02 View deployment

PAkerstrand marked this pull request as ready for review June 29, 2022 12:58

PAkerstrand requested review from trueadm, zurfyx, fantactuka, acywatson, tylerjbainbridge and thegreatercurve as code owners June 29, 2022 12:58

PAkerstrand changed the title ~~PoC: capture the expected payload type in commands~~ Capture the expected payload type in commands Jun 29, 2022

thegreatercurve approved these changes Jun 29, 2022

View reviewed changes

thegreatercurve closed this Jun 29, 2022

thegreatercurve reopened this Jun 29, 2022

zurfyx reviewed Jun 29, 2022

View reviewed changes

packages/lexical/src/LexicalEditor.ts Outdated Show resolved Hide resolved

packages/lexical-plain-text/src/index.ts Show resolved Hide resolved

PAkerstrand commented Jun 29, 2022

View reviewed changes

packages/lexical/src/LexicalEditor.ts Outdated Show resolved Hide resolved

vercel bot deployed to Preview – lexical June 30, 2022 07:08 View deployment

vercel bot deployed to Preview – lexical-playground June 30, 2022 07:09 View deployment

PAkerstrand commented Jun 30, 2022

View reviewed changes

vercel bot deployed to Preview – lexical June 30, 2022 07:22 View deployment

vercel bot deployed to Preview – lexical-playground June 30, 2022 07:23 View deployment

vercel bot deployed to Preview – lexical June 30, 2022 10:40 View deployment

vercel bot deployed to Preview – lexical-playground June 30, 2022 10:41 View deployment

PAkerstrand commented Jun 30, 2022

View reviewed changes

PAkerstrand added 8 commits July 11, 2022 10:29

refactor(core): capture the expected payload type in commands

1b97bef

fix(core): update payload type of CONTROLLED_TEXT_INSERTION_COMMAND

6fb3bc5

Previously typed as `string`, updated to `InputEvent | string`

feat(core): add union types to PASTE_, COPY_ and CUT_COMMAND

1c7cee2

feat(plain-text): handle event types in onCopy/Cut/PasteForPlainText

ddad5f3

feat(rich-text): handle event types in onCopy/Cut/PasteForPlainText

10e071b

fix: check for Keyboard/InputEvent instead of ClipboardEvent

9fa3925

The `jsdom`-environment in Jest does not have `ClipboardEvent` globally defined, causing the unit tests to fail

refactor: use empty interface to capture command payload generic

684c24e

Additionally, use generic contraints to enforce the command type and payload type to match. Fix type errors from stricter enforcements introduced

fix: use Record<string,never> for empty object

2443b9b

add some TSDoc for `CommandPayloadType`-helper

thegreatercurve force-pushed the refactor-infer-payload-type-in-command branch from 8b6e8b2 to 2443b9b Compare July 11, 2022 09:30

vercel bot deployed to Preview – lexical-playground July 11, 2022 09:32 View deployment

thegreatercurve merged commit 04a8b55 into facebook:main Jul 11, 2022

thegreatercurve mentioned this pull request Jul 11, 2022

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

Closed

PAkerstrand deleted the refactor-infer-payload-type-in-command branch July 12, 2022 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture the expected payload type in commands #2537

Capture the expected payload type in commands #2537

PAkerstrand commented Jun 27, 2022 •

edited

Loading

vercel bot commented Jun 27, 2022 •

edited

Loading

thegreatercurve Jun 29, 2022

acywatson Jun 29, 2022 •

edited

Loading

PAkerstrand Jun 29, 2022

thegreatercurve commented Jun 29, 2022

thegreatercurve commented Jun 29, 2022

zurfyx left a comment

PAkerstrand Jun 30, 2022

PAkerstrand Jun 30, 2022

PAkerstrand Jun 30, 2022

PAkerstrand Jun 30, 2022

PAkerstrand Jun 30, 2022

PAkerstrand commented Jun 30, 2022

PAkerstrand Jun 30, 2022

PAkerstrand commented Jun 30, 2022

acywatson commented Jul 4, 2022 •

edited

Loading

PAkerstrand commented Jul 5, 2022

	event: CommandPayloadType<typeof COPY_COMMAND>,
	event: ClipboardEvent \| KeyboardEvent,

Capture the expected payload type in commands #2537

Capture the expected payload type in commands #2537

Conversation

PAkerstrand commented Jun 27, 2022 • edited Loading

vercel bot commented Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

acywatson Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegreatercurve commented Jun 29, 2022

thegreatercurve commented Jun 29, 2022

zurfyx left a 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

PAkerstrand commented Jun 30, 2022

Choose a reason for hiding this comment

PAkerstrand commented Jun 30, 2022

acywatson commented Jul 4, 2022 • edited Loading

PAkerstrand commented Jul 5, 2022

PAkerstrand commented Jun 27, 2022 •

edited

Loading

vercel bot commented Jun 27, 2022 •

edited

Loading

acywatson Jun 29, 2022 •

edited

Loading

acywatson commented Jul 4, 2022 •

edited

Loading