Skip to content

Conversation

deepeshhmehta
Copy link

Hi again,

Added one definition (may need addition from your end like last time).

Also added support for multiple onRequest hooks.

(we are generating the config object programatically and there are scenarios where we would appreciate this functionality)

My first thought was to override the existing onRequest attribute of Hooks but then i thought it would break existing functionality

@jkyberneees
Copy link
Collaborator

Hi @deepeshhmehta thanks for taking the time to improve fast-gateway. Also, excuse me for getting a bit late to this conversation.

I have carefully read your proposal and I can understand and appreciate the utility on chaining hooks to achieve cleaner and more maintainable code.

However, this implementation would not be aligned with the rest of the supported hooks, who are tied to fast-proxy API.
Can I propose you to implement this chaining logic into a separated module? For example:

const hooksChain = require('fg-hooks-chain')

...
onRequest: hooksChain([hook1, hook2, hook3])
...

The implementation could also cover other hook types as well.

In this way, any custom hooks strategy would remain out of the core, allowing devs to decide whether or not they want to extend the built-in support.

Best Regards,
Rolando

@deepeshhmehta
Copy link
Author

Alright, sounds good.

  • I shall deploy a plugin to npm for hooksChain
  • I shall edit this PR to
    • add a demo.js consuming the new hooks plugin I'll deploy.
    • remove the hooksChain logic
    • I would still like to keep the Opts definittion

@jkyberneees
Copy link
Collaborator

You are great, looking forward to hearing more from you on this topic.

Deepesh Mehta added 2 commits June 22, 2020 19:07
Added a demo app consuming multiple hooks
@deepeshhmehta
Copy link
Author

@jkyberneees Please review the PR, added changes as per our discussion.

@deepeshhmehta deepeshhmehta changed the title Added functionality to support multiple hooks Added Demo to support multiple hooks Jun 23, 2020
@jkyberneees
Copy link
Collaborator

Thanks @deepeshhmehta, great work. I will merge accordingly.
As a final recommendation, It would be great if you can add proper tests for fg-multiple-hooks project.

Regards

@jkyberneees jkyberneees merged commit 7f39e79 into BackendStack21:master Jun 23, 2020
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