Events only allow one handler to be executed before returning

[Pearce-Ropion]

Describe the bug
The new event handlers only allow one event handler to be executed before returning
https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-core/src/lib/registry.js#L231

Expected behavior

If multiple event handlers are registered for the same event, each should be executed in the order they were registered

Based on this line, I would assume multiple event handlers would be allowed
https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-core/src/lib/registry.js#L223

Applicable Versions:

• Netlify CMS App version: [e.g. 2.13.2
• Git provider:Gitlab
• OS: Linux Debian 10

• Node.JS version: Attempted on 10, 12 14

[erezrokah]

Thanks @Pearce-Ropion, good catch. This was done to support #3812.
I think we should can choose to use the last value in that case, WDYT?

[Pearce-Ropion]

Thanks @Pearce-Ropion, good catch. This was done to support #3812.
I think we should can choose to use the last value in that case, WDYT?

What about saving each progressive value?

export async function invokeEvent({ name, data }) {
    validateEventName(name);
    const handlers = registry.eventHandlers[name];

    let _data = data;
    for (const { handler, options } of handlers) {
        try {
            _data = await handler(_data, options);
        } catch (e) {
            console.warn(
                `Failed running handler for event ${name} with message: ${e.message}`
            );
        }
    }
    return _data;
}

[erezrokah]

What about saving each progressive value?

Sure that's even better

[Pearce-Ropion]

@erezrokah
So I started on this change however, I ran into an issue for the preSave event where the fn should return a modified DataMap however, the event handler is expecting an EntryMap as a parameter. To solve this we would have to do something like the following. However this would mean that invokeEvent is no longer agnostic to the data parameter passed to it. Is this ok?

// Using the interface to convey the types easily but wont actually put this in the code since this file is not typed
interface InvokeEventParams {
  name: string;
  data: EntryMap;
}

export async function invokeEvent({ name, data }: InvokeEventParams): Promise<DataMap> {
    validateEventName(name);
    const handlers = registry.eventHandlers[name];

    let _data = data;
    for (const { handler, options } of handlers) {
        try {
            const result = await handler(_data, options);
            _data = _data.set('data', result);
        } catch (e) {
            console.warn(
                `Failed running handler for event ${name} with message: ${e.message}`
            );
        }
    }
    return _data.get('data');
}

---

Alternatively, we could change invokePreSaveEvent to return a EntryMap (currently returns a DataMap)

async invokePreSaveEvent(entry: EntryMap): EntryMap {
  return await this.invokeEventWithEntry('preSave', entry);
}

And change this line, to update the data from the returned entry
https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-core/src/backend.ts#L1052

const modifiedEntry = await this.invokePreSaveEvent(draft.get('entry'));
const entryDraft = (modifiedEntry && draft.setIn(['entry', 'data'], modifiedEntry.get('data'))) || draft;

However, this would be a breaking change to anyone that currently using the preSave event to modify data as they would have to modify their handler to return the full entry.

What are your thoughts?

[erezrokah]

However this would mean that invokeEvent is no longer agnostic to the data parameter passed to it. Is this ok?

I think this is ok

However, this would be a breaking change to anyone that currently using the preSave event to modify data as they would have to modify their handler to return the full entry.

This feature is still in Beta, so technically we could break it... I would still go with the first option.