Feature request: Add a 'checkLocation' option to the Suggest plugin

[samwillis]

Currently the suggest plugin triggering is controlled by the char/startOfLine options and so it is triggered in any text node. It would be brilliant If we could limit-to/exclude-from certain nodes.

Something like this would work:

// Exclude the suggestions from blockquotes
checkLocation: (editor) => {
    return !editor.commands.isActive('blockquote')
}

I'm using it for hashtags and have a node type where hash tags are not supported.

[oodavid]

I second this.

Maybe it could be best to have schema-checking instead? That's the source of truth for valid structures.

[samwillis]

Agreed, except for decorations and running arbitrary commands. I'm using it with a hashtag extension (going to post it now) where the hashtags are just hi-lighted with a decoration rather than being a node themselves.

[samwillis]

Completely untested attempt:

Change:
https://github.com/ueberdosis/tiptap-next/blob/13ad3acf632cc25a4f0c801804188b4c20db1f9f/packages/suggestion/src/suggestion.ts#L152
to:

          if (match && checkLocation(editor)) {

Add to export interface SuggestionOptions

checkLocation?: (editor: Editor) => boolean,

and add export function Suggestion default args:

checkLocation = () => true,

[philippkuehn]

Great idea! I’ve implemented this in the feature/allow-option-for-suggestions branch with this commit.

But I’ve run into a problem that I haven’t been able to fix yet: circular types (editor as any is a quick fix). This is a major blocker for me and I absolutely don’t know how to fix it. Does anyone have any ideas? :(

[oodavid]

Can you walk us through the nuances of the circular issue? Maybe a video could work?

[philippkuehn]

@oodavid There are a bunch of TypeScript errors:

This is due to the fact that all extensions are registered in the AllExtensions interface to extract its commands. These are accessible via editor.commands and as soon as editor is used in a few places within an extension, the circular type issue occurs and everything explodes.

[samwillis]

From a quick bit or reading it sounds like the solution to circular types is to create a new interface, see microsoft/TypeScript#3496 (comment)

Apparently "this works because resolution of interface base types and interface members is deferred, whereas resolution of type aliases is performed eagerly"

So something like this?

// Somewhere, maybe at the top of the file or have it shared for use in multiple plugins
interface EditorObject extends Editor { }

// Then do:
  allow?: (props: {
    editor: EditorObject,
    range: Range,
  }) => boolean,

I have not tested this...

[philippkuehn]

@samwillis Thanks but I can’t get it to work with that :(

[samwillis]

Thats frustrating! It always seems clever tools are brilliant until you get that edge case where they fall over. I have been doing some more looking and I think this describes a possible solution in this case: https://stackoverflow.com/questions/61259112/how-to-solve-circular-dependencies-when-useing-classes-as-types-in-typescript

I think you may need to create a Editor.types.ts or Editor.d.ts file that describes the Editor interface (which the editor then implements), that can then be imported into the plugins rather than the Editor.ts file itself (breaking that circular loop). Would happily help but forking is disabled and so wouldn't be able to create a pull request.

As someone new to typescript (only started using it a few months ago) having mostly worked in the dynamic world of Javascript and Python and so being suspicious of the "overhead" of a typed language I must say I'm loving the way it integrates with VS Code, it so quick to spot (potential) bugs.

[oodavid]

The MobX author / core contributor has this article on a seemingly simple fix for circular dependencies.

You can probably skip the preamble / examples and jump to "The internal module pattern to the rescue!"

https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de

[philippkuehn]

@oodavid

The MobX author / core contributor has this article on a seemingly simple fix for circular dependencies.
You can probably skip the preamble / examples and jump to "The internal module pattern to the rescue!"
https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de

I tried that pattern a few weeks ago without any luck :(

@samwillis

I think you may need to create a Editor.types.ts or Editor.d.ts file that describes the Editor interface (which the editor then implements), that can then be imported into the plugins rather than the Editor.ts file itself (breaking that circular loop).

Should be worth a try.

Would happily help but forking is disabled and so wouldn't be able to create a pull request.

Unfortunately GitHub doesn’t allow forking on private repos. But I gave you write permissions. With this it should also work. I’m really grateful for any help, because this problem has been with me for months now. tiptap v2 is also my first TypeScript project 😬

[samwillis]

No worries, its a brilliant project and happy to get stuck into an interesting problem. Will help me learn more about TypeScripts intricacies. I'm working though the circular dependancies at the moment... will let you know what I come up with.

[samwillis]

Right, I think I sort of have my head around it. The issues (at least in this case) is a result of directly returning the end of a command chain. The short version is that this works:

      allow: ({ editor, range }) => {
        if (editor.can().replaceRange(range, 'mention')) return true;
        return false;
      },

Interestingly this doesn't work:

      allow: ({ editor, range }) => {
        return !!(editor.can().replaceRange(range, 'mention'))
      },

The longer version is command chains are (and have to be) recursive types, you have implemented them here (which I'm still getting my head around but getting there):

https://github.com/ueberdosis/tiptap-next/blob/2bd30332078d746194741680f2ef0c5af45d5233/packages/core/src/types.ts#L129-L137

When I experimented with breaking the recursive nature of this it means you loose all types on the command chains, obviously not a solution. So my thinking is can we add some sort of .end() to the chain to stop the recursive type reference? .run() on a .chain() does not do this, it causes the same problem as a .can() chain.

Before coming to that conclusion, I tried creating interface definitions for Editor, Node, Mark and Extension and using them anywhere that could be in the dependancy chain. I also tried replacing some of the types in the types files with interfaces inheriting from a generic as that is apparently one of the tricks to break a recursive type. Both help as its the command chain that is explicitly recursive.

[samwillis]

A quick thing to add, I tried changing this:
https://github.com/ueberdosis/tiptap-next/blob/2bd30332078d746194741680f2ef0c5af45d5233/packages/core/src/CommandManager.ts#L77-L83

to:

        if (name === 'run') {
          if (!hasStartTransaction && shouldDispatch && !tr.getMeta('preventDispatch')) {
            view.dispatch(tr)
          }

          return () => {
            if (callbacks.every(callback => callback === true)) return true;
            return false;
          }
        }

And it didn't work (it was a bit of a long shot), so I think what is needed is making the typescript compiler recognise that run() or an end() is the end of the type, which brings us back to ChainedCommands and CanCommands in types.ts

[philippkuehn]

Hey, thanks for diving in! I think the issue is somewhere else (not explicit in CanCommands). There is another place that leads to the circular type error:

Change in the NodeConfig interface this:

  addKeyboardShortcuts?: (this: {
    options: Options,
    editor: Editor,
    type: NodeType,
  }) => {
    [key: string]: any
  },

to this:

  addKeyboardShortcuts?: (this: {
    options: Options,
    editor: Editor,
    type: NodeType,
  }) => {
    [key: string]: () => boolean
  },

The second one should be the correct one but then everything explodes again 🙃

[philippkuehn]

A quick thing to add, I tried changing this:
https://github.com/ueberdosis/tiptap-next/blob/2bd30332078d746194741680f2ef0c5af45d5233/packages/core/src/CommandManager.ts#L77-L83

to:

        if (name === 'run') {
          if (!hasStartTransaction && shouldDispatch && !tr.getMeta('preventDispatch')) {
            view.dispatch(tr)
          }

          return () => {
            if (callbacks.every(callback => callback === true)) return true;
            return false;
          }
        }

And it didn't work (it was a bit of a long shot), so I think what is needed is making the typescript compiler recognise that run() or an end() is the end of the type, which brings us back to ChainedCommands and CanCommands in types.ts

I think any JavaScript changes in here doesn’t matter because I force the return type of createChain as ChainedCommands because TypeScript can’t get the types of Proxies:

https://github.com/ueberdosis/tiptap-next/blob/2bd30332078d746194741680f2ef0c5af45d5233/packages/core/src/CommandManager.ts#L100

[philippkuehn]

But maybe that proxy is the problem and we have to find a way to get createChain working with an explicit run function that ends the chain.

[samwillis]

It may well be, I suppose there is also the proxy wrapping the whole editor instance too, which could impact the issue in the addKeyboardShortcuts interface, otherwise that looks slight more odd?

I wander if there is a clue to how the TypeScript compiler is interpreting the type with the way it acts differently with:

allow: ({ editor, range }) => {
  return !!(editor.can().replaceRange(range, 'mention'))
},

and

allow: ({ editor, range }) => {
  if (editor.can().replaceRange(range, 'mention')) return true;
  return false;
}

with the later working...

Just checked and these also don't work:

return (editor.can().replaceRange(range, 'mention') === true)
return editor.can().replaceRange(range, 'mention') ? true : false

Its almost like a branch forces a new type...

[samwillis]

Pretty confident its not confined to chains, I commented out ChainedCommands and CanCommands and tried this:

command: ({ editor, range, props }) => {
  return editor.commands.insertText(' ')
}

and it fell over in SingleCommands/AllCommands, so it's not to do with the chain proxy.

[philippkuehn]

Hmm, then it’s maybe the way how I read the commands from the AllExtensions interface :(

addCommands is the only value that is inferred from the extensions. I wanted to avoid unnecessary type declarations here. But maybe we have to do something like that instead:

Extension.create({
  addCommands() {
    return {
      whatever: (someProp) => ({ editor }) => {
        // ...
      }
    }
  },
})

declare module '@tiptap/core' {
  interface AllCommands {
    whatever: (someProp: any) => Command,
  }
}

[samwillis]

That may well work but is obviously not ideal, that's actually the really clever bit.

I'm suspicious that Extension/Commands are circular types by nature and which as we know typescript doesn't support. My thinking (again, based upon relatively limited experience and so I could be wrong) it that because you can call other extensions commands from within commands, as soon as you return a value directly (not forcing a brach) from a command within another command you risk creating a circular inferred type.

Because keyboard shortcuts are often returning a value from a commend (to indicate success and stop trying other shortcuts) they inherently create a circular type too, by doing the below you force Typescript to stop following the circular nature:

  addKeyboardShortcuts?: (this: {
    options: Options,
    editor: Editor,
    type: NodeType,
  }) => {
    [key: string]: any
  },

I don't think there is a going to be a perfect solution, at best what is needed is something that means that users of TipTap don't accidentally create a "circular type" error themselves but still get all the nice developer experience with the typed commands on the editor instance.

[philippkuehn]

I have tested a bit and it seems to work if the commands are not inferred (tested the shortcuts issue). Maybe we should remove that "magic" part :/

It’s not too bad to use it like so:

import { Command } from '@tiptap/core'

Extension.create({
  addCommands() {
    return {
      simpleCommand: () => ({ editor }) => {
        // ...
      },
      commandWithProps: props => ({ editor }) => {
        // ...
      },
    }
  },
})

declare module '@tiptap/core' {
  interface Commands {
    simpleCommand: () => Command,
    commandWithProps: (props: any) => Command,
  }
}

Using the commands works as before.

There are also some nice side effects. Since we don’t have two generics for the ExtensionConfig anymore (Options and inferred Commands before), we can pass the options directly to the extension:

Extension.create<Options>({
  defaultOptions: {
    // ...
  },
})

instead of

Extension.create({
  defaultOptions: <Options>{
    // ...
  },
})

which was already some kind of "hack" and has some downsides.

There are also plans to extend the NodeConfig and MarkConfig interfaces to add some custom schema values. This is already used for tables:

https://github.com/ueberdosis/tiptap-next/blob/0ed368e9f4617982a98cf7c9ebb180e59956b913/packages/extension-table/src/table.ts#L59

We had to add tableRole to the NodeConfig interface for now but this is not ideal because it can be confusing if you don't use tables at all.

Extending these interfaces will be much easier if we don't have inferred generic types.

[samwillis]

That doesn't sound like its all bad then.

If I'm following right, does that mean it would be possible to add a way to create extensions that add additional configuration options to existing nodes (by extending the NodeConfig interface)? So would it be possible to create a Gestures extension that could be configured something like this (inspired by the gestures in the iOS Notes app):

const editor = new Editor({
  extensions: [
    Document,
    Paragraph,
    Text,
    OrderedList,
    ListItem.extend({
      addGestures() {
        return {
          'Swipe-Left': () => this.editor.commands.sinkListItem(),
          'Swipe-Right': () => this.editor.commands.liftListItem(),
        }
      },
    },
    Gestures,
  ],
})

This would obviously need an API for the Gestures extension to use to add its support to nodes.

[philippkuehn]

Not in the first step. First I would like to be able to register custom fields within the NodeSpec/MarkSpec. As a next step one could then register more complex things, like helper methods or gestures. In the end it could be possible to register the whole logic of commands via an extension.

But you can imagine how much work it will be to mockup an API that registers more APIs. Especially with TypeScript. 😅

[philippkuehn]

Hey, a little update from me. After I made the planned changes in this branch, I noticed a problem, which is not insignificant.

If you create an extension with a command like this …

declare module '@tiptap/core' {
  interface Commands {
    foo: (param: number) => Command,
  }
}

const myExtension = Extension.create({
  addCommands() {
    return {
      foo: (param) => () => {
        // ...
      },
    }
  },
})

and want to extend it later with overwriting its command …

declare module '@tiptap/core' {
  interface Commands {
    foo: (param: number, param2: string) => Command,
  }
}

const extendedExtension = myExtension.extend({
  addCommands() {
    return {
      foo: (param, param2) => () => {
        // ...
      },
    }
  },
})

TypeScript will throw an error:

Subsequent property declarations must have the same type. Property 'foo' must be of type '(param: number) => Command', but here has type '(param: number, param2: string) => Command'.ts(2717)

And I have no idea to prevent this :(

[samwillis]

I suppose that is exactly that TypeScript is supposed to do in that situation as you are trying to change the type signature of an already defined command? Is that affecting something already in the codebase?

Does the trick described here work: https://www.damirscorner.com/blog/posts/20190712-ChangeMethodSignatureInTypescriptSubclass.html

[philippkuehn]

@samwillis Me again. I found a way to overwrite commands types. What you have to do is to add a unique scope to your command declaration:

declare module '@tiptap/core' {
  interface Commands {
+   myExtension: {
      foo: () => Command,
+   }
  }
}

With that you could overwrite a command type:

declare module '@tiptap/core' {
  interface Commands {
    myExtension: {
      foo: () => Command,
    }
  }
}

const myExtension = Extension.create({
  name: 'myExtension',

  addCommands() {
    return {
      foo: () => () => {
        return true
      },
    }
  },
})

// overwrite `foo` command

declare module '@tiptap/core' {
  interface Commands {
    myExtendedExtension: {
      foo: (name: string) => Command,
    }
  }
}

const myExtendedExtension = myExtension.extend({
  addCommands() {
    return {
      // typeof name = string
      foo: (name) => () => {
        return true
      },
    }
  },
})

But there is one downside where I'm not sure we should ignore it. But that's in the nature of overwriting. Imagine the following command:

declare module '@tiptap/core' {
  interface Commands {
    myExtension: {
      foo: (attribute: boolean) => Command,
    }
  }
}

const myExtension = Extension.create({
  name: 'myExtension',

  addCommands() {
    return {
      // error: command should return `boolean`
      // `attribute` is now type of `string` because of overwriting
      foo: attribute => () => {
        return attribute
      },
    }
  },
})

// overwrite `foo` with different props

declare module '@tiptap/core' {
  interface Commands {
    myExtendedExtension: {
      foo: (attribute: string, attribute2: boolean) => Command,
    }
  }
}

const myExtendedExtension = myExtension.extend({
  addCommands() {
    return {
      foo: (attribute, attribute2) => () => {
        return attribute2
      },
    }
  },
})

This could only happen if we change types of existing attributes. In other situations this is not a problem:

declare module '@tiptap/core' {
  interface Commands {
    myExtension: {
      foo: (name: string) => Command,
    }
    myExtendedExtension: {
      // no problem
      foo: (name: string, attributes: { [key: string]: any }) => Command,
    }
  }
}

In the situations where this leads to problems, you could of course still register commands under a new name. What do you think?

[samwillis]

I don't think it's a problem.

I can see that there may be occasions where adding an arg to an existing command is needed but changing the type of an arg will only lead to bugs as it could break other plugins that rely on the original signature and functionality (exactly what typescript is there to help prevent). I think if you need to make a command perform significantly differently it should be under a new name to avoid any conflicts.

[philippkuehn]

Ok, merged and published. Uff! Biggest change for a long time.
I also removed all proxies in that move (maybe something is better now for Vue 3?)

[samwillis]

Brillant! I bet that was a bit of a mission... Thank you!

Only thing I have spotted after upgrading is I had a few custom keyboard shortcuts where I was excepting the state as the first arg ('Mod-Enter': (state: any) => {). This now throws a typescript error, not a problem as I have changed it to just do this.editor.state but I think from memory you are actually passing on the args (EditorState, dispatch, EditorView) from the prosemirror keymap?

I have just tested for my original bug with Vue3 ("Applying a mismatched transaction" error when trying to run a command from a button), it unfortunately still happens when returning the editor as a data attribute or wrapped in a ref() (effectively the same), so it doesn't look like it was an issue with the proxies after all. It's looking more like that my subclass with the reactive methods may be needed for Vue 3. I will do some more digging and see what I can find out about how the new reactive system is working.

[philippkuehn]

Ah totally missed to fix that type. Should be fixed right now!

I already thought that with Vue 3. But the proxies were still unnecessary in the meantime.