Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/prioritised eventifier #179

Closed
wants to merge 2 commits into from

Conversation

oatymart
Copy link
Contributor

Related to: https://oat-sa.atlassian.net/browse/PISA25-621

In a customer test runner plugin, we found a situation where 2 runner.before('move') handlers, attached to the runner by 2 distinct plugins, needed to be executed in a guaranteed order. [discussed here]

The proposed solution is to turn eventifier's handler array into a kind of priority queue - in the most simple way possible (instead of always Array.push, it uses Array.splice).

Use eventifier like so, and you can guarantee the order of handler execution:

function earlierHandler(e) { console.log(1, e); }
function laterHandler(e) { console.log(2, e); }
earlierHandler.priority = 7;
laterHandler.priority = 3;
emitter.on('hello', laterHandler); // attached 1st, called 2nd
emitter.on('hello', earlierHandler); // attached 2nd, called 1st
emitter.trigger('hello', 'world');

Use it without priority and it behaves as before (Array.splice behaves like Array.push).

To test: npm run test

Copy link

Version

Target Version 3.1.0
Last version 3.0.0

There are 0 BREAKING CHANGE, 1 feature, 0 fix

@olga-kulish olga-kulish self-requested a review November 23, 2023 13:06
Copy link
Contributor

@olga-kulish olga-kulish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this is useful. It also doesn't do any harm: don't need it - don't use it.

But let's go forward with existing solution for customer ticket, because this requires more changes.

if (insertionIndex === -1) {
insertionIndex = handlers.length;
}
handlers.splice(insertionIndex, 0, handler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not enough, because triggerBefore/triggerHandlers still executes all handlers in parallel (Promise.all(pHandlers)).

But it should do Promise.all only for handlers of the same priority, and also not execute next-priority if higher-priority cancelled this event.

As for who sets priority, I think we can start from hardcoding it in customer plugin, but logically test-runner should manage that, maybe by updating config option (+ inside some dynamicModulesIndex util to allow customer repo override).

---- test-runner qti.js install/init
<checks which plugins are enabled>
config.options.plugin1.beforeMoveHandlerPriority = 0;
config.options.plugin2.beforeMoveHandlerPriority = 1;

---- plugin-1
handler.priority = config.options.plugin1.beforeMoveHandlerPriority || 0
.before('move', handler)

---- plugin-2
handler.priority = config.options.plugin2.beforeMoveHandlerPriority || 0
.before('move', handler)

@olga-kulish olga-kulish marked this pull request as draft November 27, 2023 12:55
@oatymart oatymart closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants