hooks for signin & signout

[gr2m]

follow up for discussion at https://github.com/hoodiehq/hoodie-client/issues/42#issuecomment-169656334. It’s worth reading to get some background what we try to achieve and why.

so what we want is this

hoodie.account.on('signin', function (account, options) {
  options.hooks.push(function () {
    return Promise.reject(new Error ('foo'))
  })
})
hoodie.account.signIn({username: 'pat', password: 'secert')
.catch(function (error) {
  // fails with "foo" error
})

@danreeves came up with the idea and made a reference implementation of hooks option here: https://github.com/hoodiehq/hoodie-client/compare/master...danreeves:hoodie-reset

---

• [x] claim this issue (@gr2m)
• [x] start a PR #76
• [x] add integration tests for before:signin / after:signin events
• [x] implement before:signin / after:signin events
• [x] add integration tests for before:signout / after:signout events
• [x] implement before:signout / after:signout events
• [x] review (@danreeves)
• [x] merge
• [x] update https://github.com/gr2m/milestones/issues/88

[patriciagarcia]

I can take this one :metal:

A couple of questions:

• from https://github.com/hoodiehq/hoodie-client/issues/42:

Use cases I can think of for hooks are

2. account.signout: push local data before, clear local data after

So we still need pre and post hooks, not just to intercept a signin or signout or am I missing something? "Intercept" sounds like a pre hook to me.

• wouldn't it be useful to allow to remove the hooks? not only add them? This is mentioned in the other issues but not anymore in this one.
• if there are several hooks for the same event, in which order will they be called? If we don't care about this, should we explicitly say in the README that the order of execution is not guaranteed?

[gr2m]

So we still need pre and post hooks, not just to intercept a signin or signout or am I missing something? "Intercept" sounds like a pre hook to me.

Hmmmm that’s a tough one. I somehow thought we had this figure out but now it’s puzzling again. Ideally, we would have hooks that are always run before the event, then we could clear the store on the event itself.

So maybe we should add these hooks to special before:signin, before:signout, before:signup events after all? That would make the order of hooks / events more clear, too, maybe?

wouldn't it be useful to allow to remove the hooks? not only add them? This is mentioned in the other issues but not anymore in this one.

The hooks happen directly in the event handlers, they are not persisted. So if you unsubscribe from the events, the hooks won’t happen any more.

if there are several hooks for the same event, in which order will they be called? If we don't care about this, should we explicitly say in the README that the order of execution is not guaranteed?

See maybe comment above. But in general yes, we should document how this works in README. Because either way the order of how the hooks are run is not guaranteed

[patriciagarcia]

Hey! Thanks for the comments, a couple of questions still (sorry! :grin:):

Hmmmm that’s a tough one. I somehow thought we had this figure out but now it’s puzzling again. Ideally, we would have hooks that are always run before the event, then we could clear the store on the event itself.

You mean that the store will be cleared by default in the event code itself, without the need of adding a hook? Otherwise, with the before hooks we can clear the store before the signout, the problem is that if the signout fails after all, the store would have been anyway cleared, which is weird.

So maybe we should add these hooks to special before:signin, before:signout, before:signup events after all? That would make the order of hooks / events more clear, too, maybe?

I like the idea, this way there is no need to remember when the hooks are executed. For me, if the event is called signout (without before:) I would assume the event is thrown after signout. Another option is to keep the events as they are and add the hooks to options.preHooks or something like that.

What if we actually keep options.preHooks and options.postHooks (maybe with better naming), would that make things much more complicated?

[danreeves]

Hi :wave: Makes sense to me to have pre and post events. I imagine it would work something like this?

hoodie.signIn () {
    var preHooks
    var postHooks

    emit('pre:signin', {hooks: preHooks})

    var signin = Promise.all(preHooks).then(function () {
        // signin stuff, returns promise
    })

    signin.then(function () {
        emit('post:signin' {hooks: postHooks})
    })

    return Promise.all(preHooks.concat(signin, postHooks));

}
``

[gr2m]

Thanks @patriciagarcia & @danreeves for the feedback, this is very valuable!

I don’t think we need post hooks really. The signout event gets trigger when the sign out succeeded, as Patricia thought. We can use that moment to clear the store. There would be no difference to do it in a post hook, because the order is not guaranteed anyway.

I think that having the before:* events as a kind of convention in Hoodie land where I as a developer can always expect to get options.hooks works good enough. I wouldn’t even document that for now to keep it internal, so we can change it later once we have more people using it.

[patriciagarcia]

Ok, let's do only the pre hooks for now then! If the post hooks are needed later they should be easy to implement.

What I am thinking now is.. do we really need the extra before:* events? Can't we just attach the hooks to the "regular" events (signin, signout, signup)? Is any other part of the code going to subscribe to the before:* events?

Not against adding them per se, just wondering that if they're not going to be used maybe it's easier just to leave them out.

[gr2m]

the only use case that comes to my mind is where we need to sync changes in before:signout and clear local store afterwards. If we would put both into the same thing, and we would need to have hooks somewhere else, then it might be that it gets executed after the store is cleared, and it could mess things up. The before:* events sound pretty clear to me, I think we should do it that way, but maybe keep it as an internal event for now, without documenting it, and see how it goes with real apps

[patriciagarcia]

But if hooks are a pre step for the signout (i.e. sign out only occurs if/when the hooks Promise succeeds) and the store is cleaned on successful signout there is no such conflict, is there?

[danreeves]

One problem I came across, and the reason I started playing with the hooks idea in the first place, was that the signout may be thenable before and code using the event has finished e.g.

hoodie.signOut().then(() => /* hoodie.store may not be cleared yet */);

Not sure if this is an expectation or not.

[patriciagarcia]

Ah, I understand.. in that case, pre-hooks could be something that is guaranteed to happen before the actual signout occurs and (potential) post-hooks could assure that something has happened before the singout event is thrown, and before signout is thenable?

Or another option, the before:signout event could be emitted before the pre-hooks are executed and (if needed) a post:signout event can be emitted after post-hooks are thenable? (signout will be emitted between the signout and the post-hooks)

[gr2m]

hoodie.signOut().then(() => /* hoodie.store may not be cleared yet */);

That is a very valid point :/ That means we need to have hooks in the signout event, too, to block the promise until the store is cleared.

So maybe we should get back to just the normal events without the before:, and have both options.preHooks and options.postHooks or sth similar? preHooks would be run before the signOut event, postHooks would be run after the signOut event, before Promise resolves?

But then the weird thing is that if postHooks fail, the signOut event woudl emit, but account.signOut() would not resolve.

Hmmmmmm

[patriciagarcia]

I was thinking of that too.. I guess it's what happens everywhere when you use post hooks? (the action occurs but the whole process: action + hooks returns an error)

If any other part of the application needs to know when/if the actual signout has happened they can use the event.

Not super nice but I can't think of anything better..

[gr2m]

This is getting more complicated, but good that we find that out before building it :+1:

My idea with having just the normal signout event won’t work, because the order the handlers are called cannot be guaranteed and a hook defined in asignout event handler cannot prevent other signout event handlers to be called. So we need the before:signout afterall I think, and option.hooks for both events. Or we implement a dedicated hook API as suggested in https://github.com/hoodiehq/hoodie-client/issues/42

[patriciagarcia]

Mm, I am lost :grin:. I think we need (if any) the after:signout event? It doesn't make sense to clear the store before the signout. My point is that hooks are either called before or after something (the signout for example), so adding the hooks to before:signout is just redundant, unless the hooks are called before the before:signout ;-).

I think we can make it work by having pre and after hooks to the signout events, resolving the signout promise only after the post hooks are executed and throwing an event when the hooks are done (after:signout? signout:after_hooks?).

I can give a try to implement this, I am sure we'll find more small details and potential problems during the implementation. If at the end we don't like the result we can go with the dedicated API but then we'll know better what is needed.

[gr2m]

How would you implement this:

The signout event should only be triggered, if all local changes have been synced, and the session ended

[patriciagarcia]

I think is possible by having local changes synced in a pre-hook, sign out (clear session) when the hook resolve and emitting the signout event, when the session is cleared.

After that post hooks are executed and when they resolve, the signout promise (hoodie.signOut().then(()) resolves and another event (after:signout?) can be emitted.

Am I missing something?

[gr2m]

okay, maybe I’m overthinking this, it’s probably time to implement something and go from there. So we just do a signout event with options.preHooks and options.postHooks, errors in preHooks prevent the actual sign out request and the signout event, postHooks can make the promise reject?

[patriciagarcia]

Yeah, that's the plan :-)

[gr2m]

okay let’s do it :+1:

[gr2m]

@patriciagarcia are you still looking into this?

[gr2m]

I’ll take it from here

[gr2m]

so my current problem is that we trigger the signin event after the request happened, but I want to be able to define hooks that prevent the request from happening in the first place.

And even if we would do it after the request happened, I definitely expect that account.username has the new value in my signin event handlers, but in my .preHooks, the username should be undefined and id should be the old one. I can’t think of a way to make this work with just signin events. I think we either need to implement hooks or do pre:signin & post:signin events, which would pass options.hooks. Not very nice, but simpler I guess.

I’m tempted to build the hooks API, but to be pragmatic, I’d go with the pre: and post: events, and not document them anywhere, so that we can change it later.

Ugh, this is hard

[gr2m]

Here are tests & implementation for pre:signin & post:signin events with options.hooks: https://github.com/hoodiehq/hoodie-client-account/pull/76/files

Looks good?

[gr2m]

okay my PR is ready: https://github.com/hoodiehq/hoodie-client-account/issues/65

Would be great if anyone of you could review it @danreeves @patriciagarcia

[danreeves]

lgtm :+1:

[danreeves]

hooray :tada:

[gr2m]

released with https://github.com/hoodiehq/hoodie-client-account/releases/tag/v2.7.0 :clap: thanks @patriciagarcia @danreeves