hooks for signin & signout about hoodie-account-client HOT 27 CLOSED

I can take this one 🤘

A couple of questions:

• from hoodiehq/hoodie-client#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?

from hoodie-account-client.

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

from hoodie-account-client.

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

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?

from hoodie-account-client.

Hi 👋 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));

}
``

from hoodie-account-client.

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.

from hoodie-account-client.

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.

from hoodie-account-client.

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

from hoodie-account-client.

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?

from hoodie-account-client.

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.

from hoodie-account-client.

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)

from hoodie-account-client.

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

from hoodie-account-client.

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..

from hoodie-account-client.

This is getting more complicated, but good that we find that out before building it 👍

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 hoodiehq/hoodie-client#42

from hoodie-account-client.

Mm, I am lost 😁. 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.

from hoodie-account-client.

How would you implement this:

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

from hoodie-account-client.

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?

from hoodie-account-client.

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?

from hoodie-account-client.

Yeah, that's the plan :-)

from hoodie-account-client.

okay let’s do it 👍

from hoodie-account-client.

@patriciagarcia are you still looking into this?

from hoodie-account-client.

I’ll take it from here

from hoodie-account-client.

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

from hoodie-account-client.

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?

from hoodie-account-client.

okay my PR is ready: #65

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

from hoodie-account-client.

lgtm 👍

from hoodie-account-client.

hooray 🎉

from hoodie-account-client.

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

from hoodie-account-client.