search

roccomuso / node-webhooks

:arrow_right_hook: Node.js module to create and trigger your own webHooks.
190 stars 47 forks source link

Change storage strategy #8

Closed oscarnevarezleal closed 1 year ago

oscarnevarezleal commented 7 years ago

Change storage strategy with a few modifications to base source. FileStorage & Redis storage are now available, new strategies are easy to integrate.

roccomuso commented 7 years ago

Just a few things, usually when there are multiple core changes, they should be previously discussed.

  1. backward compatibility (should start from 4.4.0 at least)
  2. Why did you introduce a ready() method?
  3. We should discuss an abstraction layer for the storage.
  4. good the idea of a multi() method
  5. Actually I don't like this API: const fileStorage = require('./src/storage').get('file', fileConfig) It's too verbose. Pheraps I'd suggest something like #4
oscarnevarezleal commented 7 years ago

Hi and thanks for the quick reply. I haven´t see your comments until now.

I know core changes must´ve been previously discussed.

A little background, To be honest I never intended to share the fork in first place, the changes fullfill its purpose on my project and then I tought It be a nice thing to share it with you.

2 - Storage channel isn´t always ready when attempt to use it ( Redis for instance ) 3 - Absolutely, the Json and DB references were all across the project. It was very difficult to abstract. 5 - Yeah me neither, come to it while programming usnig bottom to top approach.

roccomuso commented 7 years ago

@oscarnevarezleal we could start discussing for an abstraction layer looking for hints from other npm modules that uses multiple storage for their purposes (like express-session).

oscarnevarezleal commented 7 years ago

@roccomuso sure thing, how do you like to start?

MeStrak commented 4 years ago

Hi @roccomuso, @oscarnevarezleal I might start using this library and it would be great to be able to use redis. More generally, is the project still active?

roccomuso commented 4 years ago

@MeStrak it is but still don't officially support Redis