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

Add middlewares [Investigation] #23

Closed
vitorcamachoo opened this issue Jan 20, 2017 · 22 comments
Closed

Add middlewares [Investigation] #23

vitorcamachoo opened this issue Jan 20, 2017 · 22 comments

Comments

@vitorcamachoo
Copy link

Hii, is there any way to use cache for certains hemera services.

I'm looking for something that I've used in Express, a middleware function that caches end points.

Thanks!

@StarpTech
Copy link
Member

StarpTech commented Jan 21, 2017

Hi @vitorcamachoo caching will not be covered by hemera core. You can create a plugin which exposed a caching interface (Redis, memory ...). PR Welcome :)

hemera.add({ topic: 'globalCache', cmd: 'get', key: 'fooBar' }, ....

hemera.add({ topic: 'math', cmd: 'add' }, (req, cb) => {

 hemera.act({ topic: 'globalCache', cmd: 'get', key: 'foobar' }, (err, result) => {
    
     if(err) {
       return cb(err)
     }
     if(result) {
       return cb(null, result)
     }

     var r = req.a + req.b
     hemera.act({ topic: 'globalCache', cmd: 'set', key: 'foobar', value: r })
     cb(null, r)
  })

})

Here is a simple example how to do it actually.

hemera.add({ topic: 'math', cmd: 'add' }, function (req, cb) {

    var key = req.a + '_' + req.b

    //memory, redis ...
    if(cache[key]) {
       return cb(null, cache[key])
    }

    cb(null, req.a + req.b);
  });

@StarpTech
Copy link
Member

I close it. Feel free to reopen. Thanks.

@vitorcamachoo
Copy link
Author

Hiii,

I've been thinking about that and I'd love to have the same mechanism of ExpressJs, the possibility to have middlewares, and having that, a more generic cache could be implemented, example:

import cache from 'hemera-cache';

hemera.add({topic: 'databases', cmd: 'list'}, cache, listDatabases);

cache pseudo code:

function cache(req, res, next) {
   if(cache) res(cache) 
   else {
      save(key, response) 
      next(); 
   }
}

I could try this implementation, but I image that I will change a lot of hemera core.
I'de like to know your opinion as well.

@StarpTech
Copy link
Member

StarpTech commented Jan 27, 2017

Hi @vitorcamachoo the core should be small as possible. The design principle of hemera is to hide complexity and do only one job right. Functionality can be extended by plugins. Caching should not be land in the core.

You can write a caching wrapper on top of hemera to reach your goal. Hemera has extension there you can hook into.

Everything is an action or an implementation that's the unit of Hemera. In your example it would mean that hemera is responsible for caching (perhaps with redis...) and processing of requests that would break single responsibility.
I hope you get it. Thank you!

@vitorcamachoo
Copy link
Author

vitorcamachoo commented Jan 27, 2017

Hii, maybe I didn't explain myself clearly..
What I suggested, it's not only related with cache, is beyond that. I will try explain in a different way.

Imagine hemera have the possibility to have middlewares, like expressjs have, and with that, we can define whatever we want. The add method could be converted into this:
hemera.add(options, ...fn)
...fn could be multiple functions like you can register in express, and in that fn, it could be like that:
function(req, reply, next) {}
Where next, would pass to the next registered function (fn) in hemera.add.

This approach can open hemera to other plugins, like cache, limiter, restrict route, etc etc...
Hemera continues to do his job well, and his not part of his job to make things like cache as you said. Its the behaviour you introduce in one of these middlewares.

But as I said, it's my opinion, and if it's to implement, much of the core would be changed...

Thank you.

@StarpTech
Copy link
Member

StarpTech commented Jan 27, 2017

Hi I don't want to change the interface of add. Is your goal to define different "middlewares" per add ? Whats the difference between extension?

  hemera.ext('onClientPreRequest', function (next) {
    
    let self = this // hemera context
    next() //accept null or error object

  })

@vitorcamachoo
Copy link
Author

vitorcamachoo commented Jan 27, 2017

Of what I know, first you can't answer a request through that event, and second, that's to much generic, if I want to add a specific function behaviour as a middleware for a specific route, I can't.

Correct me if I'm wrong.

@StarpTech
Copy link
Member

StarpTech commented Jan 27, 2017

I agree that could be useful I am thinking about I will inform you.

  • global middlewares
hemera.middleware(function(req, reply, next) {
})
  • add specific middlewares
hemera.add({
  topic: 'math'
})
.middleware(function(req, reply, next) {
})

or similiar WDYT?

@vitorcamachoo
Copy link
Author

vitorcamachoo commented Jan 27, 2017

That could be a possibility, in that way, you avoid to change add method.
But, wasn't it better to apply that middleware to add method and apply a reduce to all fn registered ? I think both solutions are good, I'm just putting all the cards in the table hehe

@vitorcamachoo vitorcamachoo changed the title Ability to add cache Add middlewares [Investigation] Jan 27, 2017
@StarpTech StarpTech reopened this Jan 28, 2017
@StarpTech
Copy link
Member

StarpTech commented Jan 28, 2017

@vitorcamachoo its now possible to reply from the extension points onServerPreRequest onServerPreHandler. This was the best way without changing the core a lot.

Reply an error and finish the request i s the index of the handler

      hemera.ext('onServerPreHandler', function (next, i) {
        let self = this
        next(new Error('fail'))
      })

Reply a message and finish the request

      hemera.ext('onServerPreHandler', function (next, i) {
        let self = this
        next(null, { msg: 'unauthorized' })
      })

Reply a message and continue with the next extension handler

      hemera.ext('onServerPreRequest', function (next, i) {
        let self = this
        next(null, { msg: 'unauthorized' }, true)
      })

which informations are available you can take from the default extensions. https://github.com/hemerajs/hemera/blob/master/packages/hemera/lib/extensions.js

I think this will covering the most use cases. If you want to do preprocessing for add specific things you can do it in the add self.

@vitorcamachoo
Copy link
Author

Hii, thanks for the changes.
I've some questions about that approach.

Calling next will always end the request cycle? ie, it will never reach to the requested topic?
Since next end request cycle, I imagine that I can't broadcast data to the add event, ie, if I want to format some received data before reach the add handler. What I trying to say is, I'de like to have the same behaviour that ExpressJs have on app.use() method. I thinks this library will gain a lot with that approach.

Maybe I can create a branch to implement that and create a real complete use case for what I expect.

Thanks a lot.

@StarpTech
Copy link
Member

No calling next without passing an error object will call the next handler in the stack.

@StarpTech
Copy link
Member

StarpTech commented Jan 28, 2017

You can manipulate the request with that approach.

 hemera.ext('onServerPreRequest', function (next, i) {
      let req = this._request
      req.foo = "bar"
      next()
})

Your approach would implicit to create seperate objects for request and response. This would change the whole core but of course you can prepare something. But it must bring a lot improvements in contract to the current solution before I will update all plugins :)

@StarpTech
Copy link
Member

@vitorcamachoo so finally :)

hemera.ext('<serverExtension>', function (req, res, next, prevValue, i) {
   
   next()
})
  • next() will call the next extension handler on the stack
  • res.send(<error> or <payload>) will end the request-response cycle and send the data back to the callee but other extension point will be called.
  • res.end(<error> or <payload>) will end the request-response cycle and send the data back to the callee but no other extension point will be called.
  • req contains the current request. Its an object with two properties payload and error because decoding issues. Payload and error object can be manipulated.
  • res contains the current response. Its an object with two properties payload and error. Payload and error object can be manipulated.
  • prevValue contains the message from the previous extension which was passed by send(<value>)
  • i represent the position of the handler in the stack.

@vitorcamachoo
Copy link
Author

Cool, nice work @StarpTech.
Maybe tomorrow I will try that feature.

@StarpTech
Copy link
Member

@vitorcamachoo thanks for the PR but I will close it because I address all problems. Give me feedback thanks.

@vitorcamachoo
Copy link
Author

vitorcamachoo commented Feb 21, 2017

I change the core a little to accept what I was trying to say in old posts. You can check that in my repository. I have a little test simulating the use of a 'external lib' with that middleware, in this case, to cache response.

It isn't finished, due to the lack of time, but I would like receive your opinion.

@StarpTech
Copy link
Member

Where can I find this? Can you explain exactly what you want to solve?

@vitorcamachoo
Copy link
Author

vitorcamachoo commented Feb 21, 2017

As I said in old posts, the chance to do something like this hemera.add(options, ...fn)

I've made a simple change on add method to be able to received n handlers and web can define multiple middlewares before reach the last handler. This can be useful to manipulate data, cache data, etc etc. It's the same behavior that express have.

You can find my changes here: https://github.com/vitorcamachoo/hemera

Thanks.

@StarpTech
Copy link
Member

StarpTech commented Feb 21, 2017

HI @vitorcamachoo I like the idea that you can define middlewares per add but you forgot to update your branch. You feature cannot be merged. I did a lot improvements.

I also prefer a chaining syntax instead parameter overloading.

hemera.add({
  topic: 'test',
  cmd: 'add'
}).use(function(req, next) {
//process request
})
.end(function(req, cb) {
   cb()
})

@StarpTech StarpTech reopened this Feb 21, 2017
@StarpTech
Copy link
Member

StarpTech commented Feb 21, 2017

I create a new issue #40

@vitorcamachoo
Copy link
Author

Nice work, thanks!

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

No branches or pull requests

2 participants