paulirwin commented 2 years ago

I'm submitting a

[ ] Regression (a behavior that used to work and stopped working in a new release)
[X] Bug report
[ ] Enhancement
[X] Feature request
[ ] Documentation issue or request

Description

For reference: https://github.com/sibiraj-s/ngx-editor/blob/e6a080d6c3e498a6b56801774ca0cc892a859c70/projects/ngx-editor/src/lib/Editor.ts#L117

The default plugins are added to the plugins array before the plugins provided in options. This works fine in most non-conflicting cases, but prevents creating custom keymaps for i.e. the Enter key, or using other ProseMirror plugins that use such keymaps. The default plugins add keymap bindings earlier in the pipeline that return true preventing further keymaps from being invoked. As far as I can tell, the only way around this currently is to set keyboardShortcuts: false in the options, and re-create the default keymap plugin by copying the getKeyboardShortcuts code (since that function is not exported). I've tried editing the views plugin array after creation and that breaks things without giving an error to the console. Copying the getKeyboardShortcuts method works okay, but then you lose the ability to benefit from community improvements and bugfixes to that code with npm updates.

I wasn't quite sure whether to call this a bug report or a feature request, as it's somewhat both. As this excellent library was thoughtfully designed to be flexible and extensible, I would imagine that not being able to override keymaps by default would be an unintended bug. But at the same time, changing the order to put the default plugins last would be a breaking change, so I'm proposing a new config option to work around this issue.

I'd be happy to submit a PR with whichever solution below you feel is best, or if you have any other ideas on how to solve this problem.

Proposed Solution

Changing the order to put default plugins last (so that custom keymaps are evaluated first) would possibly be a breaking change, although if you wanted to go that route that would be fine by me.

Instead, I'd like to propose a new Options interface property of preDefaultPlugins?: Plugin[];. (Naming things is hard, I'm open to other name suggestions.) The default value for this new property in the DEFAULT_OPTIONS object would be an empty array.

A new line would be added below https://github.com/sibiraj-s/ngx-editor/blob/e6a080d6c3e498a6b56801774ca0cc892a859c70/projects/ngx-editor/src/lib/Editor.ts#L104:

const preDefaultPlugins: Plugin[] = options.preDefaultPlugins ?? [];

And then the line up above spreading the plugin array would become:

plugins: [...preDefaultPlugins, ...defaultPlugins, ...plugins],

Alternate Solution 1

Export the getDefaultPlugins, getKeyboardShortcuts, etc methods in the public API via public_api.ts. This would allow users that need custom keymaps to come first to set keyboardShortcuts: false, call getKeyboardShortcuts themselves, and add that to the array manually in whatever order they need. I think I'd prefer my proposed solution, as that doesn't have to disable and then re-enable defaults.

Alternate Solution 2

Allow for providing a callback function to build the plugins array. For example:

export type PluginsBuilder = (defaultPlugins: Plugin[]) => Plugin[];

// add to Options interface: pluginsBuilder?: PluginsBuilder

// in Editor.createEditor, something like:

    const plugins: Plugin[] = options.plugins ?? [];
    const pluginsBuilder: PluginsBuilder = options.pluginsBuilder ?? p => p;
    const attributes: Record<string, string> = options.attributes ?? {};

    const defaultPlugins = [...getDefaultPlugins(schema, {
      history,
      keyboardShortcuts,
      inputRules
    }), ...plugins];

    this.view = new EditorView(null, {
      state: EditorState.create({
        doc,
        schema,
        plugins: pluginsBuilder(defaultPlugins),
      }),
      nodeViews,
      dispatchTransaction: this.handleTransactions.bind(this),
      attributes
    });

Version Information

ngx-editor: 12.0.0
angular: 13.0.3

Browser

N/A

sibiraj-s commented 2 years ago

I'd be happy to submit a PR with whichever solution below you feel is best.

Thanks ❤️

I am slightly busy atm. Let's just do Solution 1 as a short fix. It will be just 2-3 line changes. I will release this first once the changes are done. This issue can stay open until solution 2 or any other proper fix is implemented.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

paulirwin commented 2 years ago

Thanks @github-actions bot for the reminder 😄