Disable extensions

[pierre-elie]

Another attempt at disabling extensions, related to #312

• in BlockMenu and SelectionToolbar disable items with no matching commands (provided by ExtensionManager)
• filter extensions used to build ExtensionManager based on optional prop disableExtensions?: string[]
• ⚠️ any extension can be disabled, even Doc or Text, but hey, careful what you wish for
• I tried to keep changes to a minimum and avoid disruptive changes. We could make it more user-friendly by typing the prop, but it would require some refactor of typings

[tommoor]

Niiice, this is a pretty nice pragmatic approach – typing it might be worthwhile even if it means a hardcoded array because knowing what strings are acceptable will otherwise take some digging through the codebase. Thoughts?

[pierre-elie]

Agreed typing would be great, I'm just not sure where you'd like to place the cursor between flexibility, typing and maintenance. For example in src/menus/blocks.ts, name of commands could be typed to match names of extensions.

But yeah the easiest would be a hardcoded type just for the prop

disableExtensions?: ("strong" | "em" | "highlight")[];

Slightly more involved but helping to keep this list up to date would be to type name of Marks and Nodes, for example

// src/marks/types.d.ts → declare list of acceptable mark names
export type MarkName = "strong" | "em" | "highlight"

// src/marks/Mark.ts → type name getter in abstract class
abstract get name(): MarkName;

// src/marks/Bold.ts → match return type of abstract getter
get name(): MarkName {
  return "strong";
}

// src/index.tsx
disableExtensions?: (MarkName | NodeName)[];

Ideally imo it would use the name from the class itself, without having to re-declare its type, but it can't be done if name is a getter

class Bold {
  get name () {
    return "strong";
  }
}

type BoldName = Bold["name"] // → string

Changing it to a readonly property would help (although it's not as "safe")

class Bold {
  readonly name = "strong";
}

type BoldName = Bold["name"] // → "strong"

Which then can be used elsewhere, for example

disableExtensions?: (Bold | Italic | Highlight)["name"][];

[tommoor]

@pierre-elie it sounds like you have a better understanding of Typescript than me 😄 – changing to a readonly property sounds totally fine to reduce future maintenance if you're down?

[viperfx]

This looks amazing! Can't wait for this to be in the official repo.

[pierre-elie]

Sorry didn't have time to finish. I think I'll just add hard-coded typing on the prop, which is the less disruptive change, and open another PR with "smarter" typing that requires more debatable changes. And I need to add documentation for the prop in readme.

[pierre-elie]

@tommoor ready for review :)

[tommoor]

I'm wondering what, if anything, we should do about the items that require multiple extensions to function correctly.

table, th, tr, td is a good example – if any one of these is disabled then the entire thing breaks, they come as a group 🤔

[fnwbr]

I'd say that a certain level of responsibility by the implementing developers can be safely assumed.

[tommoor]

Okay, if we have additional functionality to disable other extensions when table for example is passed then that can be an easy future change.

[dparlevliet]

This PR will error if you try to render a feature that is disabled.

Example: Without the back-end filtering out the unsupported markdown features this could lead to a crash if a user were to submit a call to the backend to have text saved that is disabled by the renderer. Or, if you just happen to have pre-existing data with features you don't want to render but still want to be readable (such as tables).

How to replicate: Add disableExtensions={ ['em'] } combined with defaultValue={ "_em_" }. This will lead to an error in the following file https://github.com/ProseMirror/prosemirror-markdown/blob/master/src/from_markdown.js#L55 - or at least it does for me.

I haven't yet found a simple fix. I have an idea but I'm still trying to get my head around the code to figure it out. Perhaps someone with more knowledge of the code base will beat me to it.

[tommoor]

Yes, it would be a good improvement to somehow disable the parser rules too – but if you're not accepting arbitrary markdown input from elsewhere then it should be relatively safe for that usecase.