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

Allow usage of a function with plugin hook, pipe and controller actions #1227

Merged
merged 17 commits into from Jan 22, 2019

Conversation

Projects
None yet
8 participants
@Aschen
Copy link
Contributor

commented Dec 21, 2018

What does this PR do ?

Allow usage of function in addition to a method name for plugin hooks, pipes and controller actions.
You can register pipes and hooks either by providing a method name or a function:

class Plugin {
 init () {
   this.hooks = {
     'core:kuzzleStart': 'printInfo', // With a method name => Deprecated
     'core:kuzzleStart': () => console.log('Hello Kuzzle!'), // With an arrow function => Recommended
     'core:kuzzleStart': this.printInfo // Not recommended, wrong function scope
   }
   this.controllers = {
     testCtrl: {
       action: 'actionHandler', // With a method name => Deprecated
       action2: async () => 'Hello', // With an arrow function => Recommended
       action3: this.actionHandler // Not recommended, wrong function scope
     }
   };
 }

 async actionHandler (request) {
   return 'hello';
 }
 
 printInfo () {
   console.log('Hello Kuzzle!');
 }
}

How should this be manually tested?

  • Step 1 : Declare a hook, a pipe and a controller action with an arrow function

@Aschen Aschen self-assigned this Dec 21, 2018

@Aschen Aschen changed the title Allow usage of a function with plugin hook and pipe in addition to a … Allow usage of a function with plugin hook and pipe Dec 21, 2018

@Aschen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Shall we depreciate the usage of a method name?

@Aschen Aschen changed the title Allow usage of a function with plugin hook and pipe Allow usage of a function with plugin hook, pipe and controller actions Dec 21, 2018

@codecov-io

This comment has been minimized.

Copy link

commented Dec 21, 2018

Codecov Report

Merging #1227 into 1-dev will increase coverage by <.01%.
The diff coverage is 95.23%.

Impacted file tree graph

@@           Coverage Diff            @@
##           1-dev   #1227      +/-   ##
========================================
+ Coverage   93.8%   93.8%   +<.01%     
========================================
  Files         97      97              
  Lines       6681    6686       +5     
========================================
+ Hits        6267    6272       +5     
  Misses       414     414
Impacted Files Coverage Δ
lib/api/core/plugins/pluginContext.js 95.93% <ø> (ø) ⬆️
lib/api/core/plugins/pluginsManager.js 86.99% <95.23%> (+0.2%) ⬆️

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 3022979...3ffdf92. Read the comment docs.

@etrousset
Copy link

left a comment

Just a comment about the PR descrption, can't we use the bind() method to bind the correct context to the callback?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Function/bind

     'core:kuzzleStart': this.printInfo.bind(this)
@Aschen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Yes we can do it like this

Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated
Show resolved Hide resolved lib/api/core/plugins/pluginsManager.js Outdated

scottinet and others added some commits Jan 14, 2019

Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Update lib/api/core/plugins/pluginsManager.js
Co-Authored-By: Aschen <amaret93@gmail.com>
Merge branch 'allow-function-in-plugin-hooks-and-pipes' of github.com…
…:kuzzleio/kuzzle into allow-function-in-plugin-hooks-and-pipes
@@ -370,7 +370,7 @@ class PluginsManager {

if (warnDelay) {
pipeWarnTimer = setTimeout(() => {
this.trigger('log:warn', `Plugin pipe ${plugin.manifest.name}:${fn} exceeded ${warnDelay}ms to execute.`);
this.trigger('log:warn', `Plugin pipe ${plugin.manifest.name}:${fn} for event '${event}' exceeded ${warnDelay}ms to execute.`);

This comment has been minimized.

Copy link
@scottinet

scottinet Jan 22, 2019

Member

This error message must be fixed: fn can now be a function. There are other occurences below to be fixed.

@kuzzle

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 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#L721: Complete the task associated to this TODO comment. rule

@scottinet scottinet merged commit 4e156a5 into 1-dev Jan 22, 2019

5 checks passed

LGTM analysis: JavaScript No alert changes
Details
codecov/project 93.8% (+<.01%) compared to 3022979
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
sonarqube SonarQube reported 1 issue, no criticals or blockers

@scottinet scottinet deleted the allow-function-in-plugin-hooks-and-pipes branch Jan 22, 2019

@benoitvidis benoitvidis referenced this pull request Jan 23, 2019

Merged

Release 1.6.0 #1238

@ScreamZ

This comment has been minimized.

Copy link

commented Mar 28, 2019

Good job

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.