Closed gr2m closed 7 years ago
I like this idea. Nice to standardize how the hoodie module come together. Having a "hook" system, as opposed to plain events, keeps the communication flexible without changing a bunch of internal stuff. For the API, to keep things consistent and similar to events, maybe we can create new hooks like an .on()
method:
// Create a hook event
hoodie.hook.on('foo'/*, method*/)
// remove pre/post hooks
hoodie.hook.off('signin', asyncPostProcessing)
It might make it a bit more approachable to remember .on()
and .off()
.
register pre/post hooks. The methods can be async by returning a Promise
If the methods return a Promise, do we chain the .then()
to the pre/post hook register?
hoodie.hook.pre('foo', asyncPreparations)
.then(/* the next step */)
Or is that only a concern for the internals of the hook
API?
If the the .off()
method uses prefixed hook names, should .on()
(or .hook()
) be able to do that as well?
hoodie.hook.on('pre:foo', asyncPreparations)
// OR
hoodie.hook('pre:foo', asyncPreparations)
With similar thoughts to @HipsterBrown couldn't you keep the normal .on methods as they already are and have npm script style pre: post:?
@HipsterBrown
// Create a hook event
hoodie.hook.on('foo'/*, method*/)
You mean .emit
here? .on
implies that you listen to something, not that you create one, wouldn’t you say? I think for defining a hook I think hoodie.hook('foo'/*, method*/)
is more clear than hoodie.hook.trigger('foo'/*, method*/)
. And "hook off" is a thing in English, right? Meaning to unhook, to disengage? I think it’s good to have the API differ a little from events, as it does something different, too
@nickcolley
couldn't you keep the normal .on methods as they already are and have npm script style pre: post:?
That was my initial thinking, but the problem is that we need more than just event handlers, we need to be able to intercept code execution flow with these hooks. So we’d need to do something like this
hoodie.account.on('pre:signout', function (options, next) {
next(hoodie.store.push())
})
It would be nice as we wouldn’t add a top level API, but I’m not sure if it’s really simpler than having a separate hook API.
@danreeves has another approach, by passing options.hooks
to events: https://github.com/danreeves/hoodie-client/commit/9ded20e918165c37b3b1f00ce04faa5c33248137
I think it’s very clever, but I’m not sure if it’s enough for what we need. Use cases I can think of for hooks are
createdBy
property should be updated, unless options.moveData = false
authorization
header for current user (we worked around this with options.ajax
for now, which is not very elegant I think: https://github.com/hoodiehq/hoodie-client/blob/master/index.js#L27-L38, but it works)Thinking ahead, there could be plugins that would need to have more hooks. For example, a share plugin would want to clear up all local data after sign out.
But if signin / signout are the only usecases we would need these hooks, and plugins wouldn’t need to define their own, then creating a dedicated hoodie.hooks
API might be overkill and options.hooks
might be better way to go
we settled for pre:*
events and options.hooks
for now, you can see them in action in lib/init.js
But I’ll leave this issue open, we might get back to it at some point later
closing in favor of https://github.com/gr2m/before-after-hook/pull/1. Please let me know what you think in the pull request even though it’s already merged <3
related: https://github.com/hoodiehq/hoodie-client/issues/22
For the glue code between hoodie.account, hoodie.store and future plugins, we sometimes need to intercept an event, for example for hoodie.reset() or to sync all local changes before signing out, so no data gets lost.
For that I’d suggest we introduce a new
hoodie.hook
API, which would look like this:So for example, to sync all local changes before sign out, we would do
If
hoodie.store.push
fails, then signout would fail. In the hoodie.account.signOut implementation we would wrap theDELETE /session
requestI was thinking about using the events API for that, but a dedicated hook API makes things more clear I think