[Audit logs] Refactor the event hub

[remidej]

What does it do?

Refactor strapi.eventHub:

• Don't use Node's events package anymore
• Instead manage our own store of callback functions that get called for every event
• Only use one single subscriber for the webhooks feature, and rely on the listeners map to decide whether a callback should run

Why is it needed?

• We'll be working with the event hub a lot for audit logs, so it's a good opportunity to work on it.
• There are memory concerns with the node event system when many listeners are attached to the same event, which may be the case for us eventually
• A similar solution has been working well for us to manage the database lifecycles

How to test it?

I tested the changes manually but we'll need to do serious QA on this at the feature level before releasing audit logs.

• Try using webhooks
• Try adding your own callback, in your app's bootstrap for example:

strapi.eventHub.subscribe((eventName, ...args) => {
  console.log(`Caught event ${eventName} with args:`, ...args);
});

[codecov]

Codecov Report

Base: 59.86% // Head: 50.06% // Decreases project coverage by -9.79% ⚠️

Coverage data is based on head (34278a7) compared to base (2a7aed6).
Patch has no changes to coverable lines.

Additional details and impacted files

@@                  Coverage Diff                   @@
##           feature/audit-logs   #15128      +/-   ##
======================================================
- Coverage               59.86%   50.06%   -9.80%     
======================================================
  Files                    1346      291    -1055     
  Lines                   32757    10270   -22487     
  Branches                 6182     2269    -3913     
======================================================
- Hits                    19609     5142   -14467     
+ Misses                  11301     4228    -7073     
+ Partials                 1847      900     -947     

Flag	Coverage Δ
back	50.06% <ø> (?)
front	?
unit	?
unit_back	50.06% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...er/services/schema-builder/content-type-builder.js	8.66% <ø> (-0.07%)	⬇️
...e/database/lib/entity-manager/regular-relations.js	9.09% <ø> (-0.11%)	⬇️
packages/core/database/lib/metadata/relations.js	19.35% <ø> (ø)
.../core/database/lib/query/helpers/populate/apply.js	3.90% <ø> (-0.09%)	⬇️
packages/core/database/lib/utils/knex.js	62.50% <ø> (-17.50%)	⬇️
...s/core/strapi/lib/services/entity-service/index.js	75.00% <ø> (+4.57%)	⬆️
...core/strapi/lib/services/entity-validator/index.js	92.48% <ø> (-1.46%)	⬇️
packages/core/strapi/lib/services/event-hub.js	100.00% <ø> (ø)
packages/core/upload/server/services/folder.js	21.68% <ø> (ø)
packages/core/upload/server/services/metrics.js	62.26% <ø> (-0.70%)	⬇️
... and 1072 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Feranchz]

🚀

[alexandrebodin]

Looks good overall. I'm wondering if we could keep the EventEmitter inheritance 🤔

[Feranchz]

I think we don't need to subscribe to all the events because now we have the on function, so instead we can go to line 63 and add this.eventHub.on(event, listen) like before this change

[remidej]

I see what you mean, but I think the webhooks is a good use case for a single subscriber function

Before there was some duplicate logic: when a webhook was added, we had to both add a listener to the map, and to add a callback with on(). And vice versa for removing a webhook

With this current solution, we have a single subscriber that directly reads the webhook map, which seems more efficient to me

[remidej]

@alexandrebodin are there other things that EventEmitter offers that we may need in the future?

My concern would be that it may cause confusion since EventEmitter exposes many methods (including many aliases), and we'd be overriding some but not all of them

[alexandrebodin]

@alexandrebodin are there other things that EventEmitter offers that we may need in the future?

My concern would be that it may cause confusion since EventEmitter exposes many methods (including many aliases), and we'd be overriding some but not all of them

That's actually the point of inheriting from a class to keep most of it's logic but extend it :)

Look at this for example https://github.com/EventEmitter2/EventEmitter2. What we want isn't a new Module but rather the old one with a way to listen to all events :) If we can avoid breaking the eventHub at all I think that is worth the shot as I know some people are already using it

[remidej]

@alexandrebodin thanks for the clarifying. The part that I just find weird about extending here is that we're only overriding on and not addListener, but in the event emitter docs they say that one is an alias for the other.

To move forward with the class extension, is it okay if I expose _listeners and _subscribers on strapi.eventHub? I though I could make them private with the # prefix but our eslint config doesn't seem to like that.

[alexandrebodin]

Please avoid refactor the webhook system if not necessary. It could benefit from a subscribe function but that's not the goal of this PR and it would require more testing if you did.

[remidej]

I understand that we want to reduce the risk by changing as little as possible, I'll update 👍

that's not the goal of this PR

Does this mean I could potentially reopen it as another dedicated PR in the future?

[alexandrebodin]

Yes of course 👍 you can if you have bandwidth for sure with added tests etc :)

[alexandrebodin]

LGTM. Good job!