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.update: set session ID from `x-set-session` response header #75

Closed gr2m closed 8 years ago

gr2m commented 8 years ago

follow up for https://github.com/hoodiehq/hoodie-client-account/pull/74#discussion_r52942960

Right now, we send an internal PUT /session request after a PATCH /session/account request which changed the account username and/or password.

Instead, we want the PATCH /session/account response to optionally include a x-set-session header which would include the new session id, so that it can be updated without sending an extra request


Working on your first Pull Request? You can learn how from this free series How to Contribute to an Open Source Project on GitHub


Ping us in the Hoodie Chat or on Twitter if you have any questions :)

steffes commented 8 years ago

I would like to take this issue

gr2m commented 8 years ago

all yours @steffes :+1:

steffes commented 8 years ago

I'm looking to see what needs to be changed in utils/request.js. Right now L19 doesn't have a body parameter and is just resolve(response) at the end. You said it is resolving just the body but it does not have a body parameter like in the testcases of nets/test.js module.

Also what is your method for debugging (i.e. stepping through with debugger) with client APIs' in general?

gr2m commented 8 years ago

is just resolve(response)

You are right, maybe it changed meanwhile, it resolves with the whole response object now, so response.headers is accessible, which is great

Also what is your method for debugging (i.e. stepping through with debugger) with client APIs' in general?

Very good question. To be honest I don’t have a good workflow for this. I’ve seen @nathanstilwell use a cool tool where you could jump in Chrome’s dev tools to debug your node code, but can’t remember what that was again? Nathan, do you know what I mean?

nathanstilwell commented 8 years ago

I've used node-inspector before with occasional success. I think it wires together the node debugger with a hook into the Chrome developer tools.

gr2m commented 8 years ago

@steffes could you give us an update?

steffes commented 8 years ago

80

gr2m commented 8 years ago

from Hoodie chat

screen shot 2016-03-06 at 21 44 54
codyzu commented 8 years ago

@gr2m from the issue description (5th checkbox)

add request headers to the DELETE /session request mock. Make sure the new session id is passed in the Authorization: bearer header. Make sure test fails when you run npm test

Why DELETE /session? I'm missing a detail or this is a copy/paste issue? Can you clarify for me :-)

gr2m commented 8 years ago

doesn’t make much sense to me, probably a copy&paste issue, ignore it :) Sorry for the confusion

gr2m commented 8 years ago

I’ll take this over from Cody, we discussed it

gr2m commented 8 years ago

Pull Request is here: https://github.com/hoodiehq/hoodie-account-client/issues/94 – if you could review that’d be ace. If things look good, comment with LGTM 👌