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

account.fetch() fails if not signed in #18

Closed gr2m closed 8 years ago

gr2m commented 8 years ago

We assume that state.session is an object here: https://github.com/hoodiehq/hoodie-client-account/blob/master/lib/fetch.js#L14

But when signed out, state.session is undefined

ianstalter123 commented 8 years ago

Trying to understand how sessions work after logout here -- does get-session.js , need to be called or something similar in fetch.js?

gr2m commented 8 years ago

in account.signOut, we clear the session here: https://github.com/hoodiehq/hoodie-client-account/blob/master/lib/signout.js#L20, which removes state.session so it becomes undefined here: https://github.com/hoodiehq/hoodie-client-account/blob/master/helpers/clear-session.js#L7

In fetch.js, we always assume that state.session is an object here: https://github.com/hoodiehq/hoodie-client-account/blob/master/lib/fetch.js#L14, but after signout, it’s not an object, but undefined

Makes sense?

So we don’t even need to send a request if state.session is undefined, and can instead fail with UnauthenticatedError as described here: https://github.com/hoodiehq/hoodie-client-account#accountfetch

We don’t have a way to handle the errors yet, so for now you can do this:

var Promise = require('../helpers/promise')
// ...
function fetch (state, basePath, path) {
  if (!state.session) {
    var error = new Error('Not signed in')
    error.name = 'UnauthenticatedError'
    return Promise.reject(error)
  }
  return request({
    url: state.url + '/session/' + basePath.replace(/\./, '/'), // replace '.' in path with '/'
    method: 'GET',
// ...

I hope that’s not too confusing. Let me know if you have any farther questions. But also don’t worry too much, I'd suggest start a pull request, and we continue discussion there?

ianstalter123 commented 8 years ago

Thanks for the explanation! So if I understand this issue should involve the error related calling fetch() when not authenticated.

gr2m commented 8 years ago

yes

gr2m commented 8 years ago

@ianstalter123 are you working on this one?

ianstalter123 commented 8 years ago

Hey yes will try to do by tonight / send a pull request

Sent from my iPhone

On Dec 11, 2015, at 12:01 PM, Gregor Martynus notifications@github.com wrote:

@ianstalter123 are you working on this one?

— Reply to this email directly or view it on GitHub.

ianstalter123 commented 8 years ago

@gr2m You want the error checking to be added to https://github.com/hoodiehq/hoodie-client-account/blob/master/lib/fetch.js#L14 right?

gr2m commented 8 years ago

Yeah I’d suggest even before the internals.fetchProperties call, check if state.session is set. And if it’s not, return a rejected Promise, similar to what we do here maybe? https://github.com/hoodiehq/hoodie-client-account/blob/master/lib/sign-in.js#L14

ianstalter123 commented 8 years ago

pull request sent

2015-12-11 13:00 GMT-08:00 Gregor Martynus notifications@github.com:

Yeah I’d suggest even before the internals.fetchProperties call, check if state.session is set. And if it’s not, return a rejected Promise, similar to what we do here maybe? https://github.com/hoodiehq/hoodie-client-account/blob/master/lib/sign-in.js#L14

— Reply to this email directly or view it on GitHub https://github.com/hoodiehq/hoodie-client-account/issues/18#issuecomment-164046385 .

gr2m commented 8 years ago

done via e577566 & 4cb4e8c :ok_hand: