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

Events triggering refactor #1308

Merged
merged 13 commits into from
May 28, 2019
Merged

Events triggering refactor #1308

merged 13 commits into from
May 28, 2019

Conversation

scottinet
Copy link
Contributor

@scottinet scottinet commented May 24, 2019

⚠️ depends on #1305

Description

Do not be afraid by the seemingly large number of changed lines, this PR is more simple than it appears.

All it does is this:

  1. it lets the kuzzle object, as an event emitter, trigger events itself (separation of concern)
  2. instead of only 1 pluginsManager.trigger method working for both hooks (simple event emissions) and pipes (awaited events returning promises, listened by plugins), we now have the standard kuzzle.emit method (synchronous, events are non-blocking), and the new kuzzle.pipe one (async, promise-based, awaited by kuzzle)

So, a little bit of code... and a looooooooooot of kuzzle.pluginsManager.trigger changed into either kuzzle.emit or kuzzle.pipe.

Expected benefits:

  1. we have a few promise leaks and I believe that this will help me fix those (this is a prereq, not a fix in itself)
  2. before this change, events that we documented as "hooks only" could actually be listened by plugins as pipes (but not awaited, which can be confusing for users)
  3. better separation of concern is always better: before it was a bit confusing since we had kuzzle as an event emitter, but with events triggered only by using the pluginsManager object

Other changes

  • removed the validation:error hook: it isn't documented, and I couldn't find a use for it

How to review

I suggest that you concentrate on reviewing the modifications made to the lib/kuzzle.js and lib/api/core/plugins/pluginsManager.js files.
Apart from unit tests updates, all other changes are anecdotical (but it's up to you obviously)

@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

Merging #1308 into 1-dev will increase coverage by 0.01%.
The diff coverage is 87.15%.

Impacted file tree graph

@@            Coverage Diff            @@
##            1-dev   #1308      +/-   ##
=========================================
+ Coverage   93.69%   93.7%   +0.01%     
=========================================
  Files          98      98              
  Lines        6934    6930       -4     
=========================================
- Hits         6497    6494       -3     
+ Misses        437     436       -1
Impacted Files Coverage Δ
lib/services/broker/index.js 100% <ø> (ø) ⬆️
...ib/api/core/entrypoints/embedded/protocols/mqtt.js 95.23% <0%> (ø) ⬆️
lib/api/core/entrypoints/embedded/context.js 70% <0%> (ø) ⬆️
lib/api/controllers/routerController.js 95.23% <100%> (ø) ⬆️
lib/services/internalEngine/bootstrap.js 90.62% <100%> (ø) ⬆️
lib/api/core/httpRouter/index.js 98.68% <100%> (ø) ⬆️
lib/api/core/entrypoints/embedded/index.js 98.31% <100%> (ø) ⬆️
lib/api/kuzzle.js 86.36% <100%> (+2.57%) ⬆️
lib/api/core/plugins/pluginContext.js 95.58% <100%> (ø) ⬆️
lib/api/controllers/collectionController.js 100% <100%> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fdb28c...9c6f323. Read the comment docs.

@kuzzle
Copy link
Contributor

kuzzle commented May 28, 2019

SonarQube analysis reported 1 issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. INFO pluginsManager.js#L759: Complete the task associated to this TODO comment. rule

@scottinet scottinet merged commit c5bcad2 into 1-dev May 28, 2019
@scottinet scottinet deleted the let-kuzzle-emit-events branch May 28, 2019 11:15
@scottinet scottinet restored the let-kuzzle-emit-events branch May 28, 2019 14:59
@scottinet scottinet deleted the let-kuzzle-emit-events branch May 28, 2019 15:10
@Aschen Aschen mentioned this pull request Jun 14, 2019
Aschen added a commit that referenced this pull request Jun 14, 2019
Release 1.8.0

Bug fixes

    [ #1311 ] Fix promise leaks (scottinet)
    [ #1298 ] Fix disabled protocol initialization (Aschen)
    [ #1297 ] Fix timeouts on plugin action returing the request (benoitvidis)
    [ #1288 ] Fix an error when trying a non-partial bulk update (scottinet)
    [ #1286 ] Allows bulk inserts on aliases (benoitvidis)
    [ #1282 ] Scan keys on redis cluster (benoitvidis)
    [ #1279 ] Users must be authenticated to use auth:logout (scottinet)

New features

    [ #1315 ] Add the new Vault module to handle encrypted application secrets (Aschen)
    [ #1302 ] Add write and mWrite (Aschen)
    [ #1305 ] Add pipes & hooks wildcard event (thomasarbona)

Enhancements

    [ #1318 ] Add a maximum ttl to auth:login (benoitvidis)
    [ #1301 ] Upgrade the WebSocket libraries (scottinet)
    [ #1308 ] Events triggering refactor (scottinet)
    [ #1300 ] Collection specifications methods cloisoned to a collection (thomasarbona)
    [ #1295 ] Improve validation error messages (benoitvidis)
    [ #1292 ] Throw an error when the realtime controller is invoked by plugin developers (benoitvidis)
    [ #1257 ] Add ability to define mapping policy for new fields (Aschen)
    [ #1291 ] Fix --help on subcommands (Yoann-Abbes)
    [ #1289 ] Handle ping/pong packets (scottinet)
    [ #1273 ] Fix incomplete access logs (scottinet)

Others

    [ #1317 ] Add ps dependency to plugin-dev Docker image for pm2 (benoitvidis)
    [ #1312 ] Check that .kuzzlerc.sample is well-formed (scottinet)
    [ #1299 ] Add Kuzzle Nightly & Redis 3 and 4 test (alexandrebouthinon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants