Potential type hole in Command type

[bradymadden97]

Describe the issue

I see the Command type is typed as (view: EditorView) => boolean

In a large codebase like Replit's it's important to share code as much as possible to avoid diverging implementations. We have a common pattern of overriding the Command type to allow us to share the underlying implementation with multiple callsites. Here's an example.

const RunCodeActionsCommand = (view: EditorView, only?: CodeActionKind [] ) => {
  ...
  return true;
}

Above is a mocked out command for running code actions. It adheres to the current Command type, as it receives an EditorView as its first parameter and returns a boolean value. But it accepts an optional second parameter that allows callsites to filter for specific CodeActionKinds.

Why is this useful? Well now, we can register RunCodeActionsCommand with keymap and it will get run without passing a only parameter; and we can call RunCodeActionsCommand with the config parameter - for instance, maybe we expose a "Fetch Refactors" option in the context menu, and we call:

RunCodeActionsCommand(view, ['refactor'])

The issue is that today I discovered CodeMirror does in fact pass a second argument when calling commands, here: https://github.com/codemirror/view/blob/4e355eab50de94ab315ed293729f5365841fe3c8/src/keymap.ts#L231

It passes a KeyboardEvent. So now my command has a typehole:

const RunCodeActionsCommand = (view: EditorView, only?: CodeActionKind[] ) => {
  if (only) {
    only.forEach( ... 
    // ^^^ this may throw, as `only` actually can be a KeyboardEvent!!!
  }
  ...
  return true;
}

I understand that what I'm doing by overriding the Command type is a bit hacky. But it seems like you could extend the Command type to warn people doing this in a backwards compatible way:

type Command = (view: EditorView, event?: KeyboardEvent<>) => boolean

Then I could just write my function interface as

const RunCodeActionsCommand = (view: EditorView, only?: CodeActionKind [] | KeyboardEvent ) => {
  if (typeof only === 'array') {
    only.forEach( ... 
    // ^^^ safe now
  }
  ...
  return true;
}

Let me know what you think! Thanks in advance.

Browser and platform

No response

Reproduction link

No response

[marijnh]

Oh, indeed. This is a general problem with the way JS/TS treat optional parameters. Attached patch avoids passing extra arguments to commands here.

[bradymadden97]

Fantastic, that works too! Thank you