hoodiehq / hoodie-account-client

:dog: Account client API for the browser
https://hoodiehq.github.io/hoodie-account-client
Apache License 2.0
11 stars 24 forks source link

WIP issue 56 add is-unauthenticated and unauthenticated event #72

Closed toh82 closed 8 years ago

toh82 commented 8 years ago

closes #54 closes #56 closes #64

toh82 commented 8 years ago

i'm not sure about the implementation of isUnauthenticated if it is really only about checking if session node exists the is-signed-in does that job already.

And i'm thinking of to put the event trigger for unauthenticated in something what is used accross the authentication methods like "sign-out, fetch" to not add the event emitter in every module, but yet donno where.

maybe i got it totally wrong and it should be like that: https://github.com/hoodiehq/hoodie-client-account/pull/62/files

gr2m commented 8 years ago

You are unauthenticated of you Are signed in, but your session is invalid

toh82 commented 8 years ago

@gr2m what means invalid in that case, no session.id given, because session object seems to exist in any case? Or should isUnauthenticated use account.validate with a new written validator, or a request for validating the session?

gr2m commented 8 years ago

yeah I agree this is confusing. Here is an example of what I mean

  1. you sign in on your phone
  2. you sign in on your laptop and change your password
  3. on your phone, you do account.fetch(), but it will fail with 401 and trigger an unauthentciate error.

I thought that the unauthenticate event would be enough, and maybe it is, I’m still not 100% sure. Problem is that when you load the page initially, you might want to show the user right away that they do not have a valid session and need to sign in again, and not make them wait until they do .fetch() again. So the .isUnauthenticated() method can be called to check that during rendering.

But how about we rename it to .hasInvalidSession(). Would that be more clear?

toh82 commented 8 years ago

Still todo:

gr2m commented 8 years ago

@toh82 what’s the state here? Are you still working on it?

toh82 commented 8 years ago

@gr2m i updated all the things \o/ and fixed an interesting bug about profile fetch meanwhile. Would be awesome if you could have a look, afterwards I'll squash it further.

I'm still sad about the code duplication in the tests, anyways, if youre fine i could make the 401 response as a utility to avoid duplication across the fetch and update modules.

gr2m commented 8 years ago

fantastic work :) If you could look into my minor comments and then clean up the commits, this is good to merge :+1:

gr2m commented 8 years ago

ah, forgot one thing, please update the README.md file, too. Add the account.hasInvalidSession() method and remove the #56 reference in the unauthenticate event

toh82 commented 8 years ago

activated integration/fetch-url-test.js and searched for more .baseUrl but found nothing, so this closes #64 too. Which commit did you meant with:

fyi that is not a feat: ..., it’s a refactor: ... as no logic is changed, just internal implementation