WIP: hoodie.request - Githubissues

hoodiehq / hoodie-client

:dog: Client API for the Hoodie server

Apache License 2.0

34 stars 25 forks source link

WIP: hoodie.request #33

Closed andru closed 8 years ago

andru commented 8 years ago

Implemented hoodie.request as outlined in the readme and #9, taking cues from hoodie-client-account in deps and code style.

Look OK?

closes #9

HipsterBrown commented 8 years ago

Looks like nets is missing from the package.json dependencies.

HipsterBrown commented 8 years ago

Looks good to me! :+1:

andru commented 8 years ago

This should be getting there I think.

options.data is now stringified
lib/request rejects with the full HTTP response as a property of the Error when status > 400
removed promise.js; lie is now required wherever we want promises, relying on it's internal check to prefer native Promises when available
a relative path is detected as a string beginning with a single forward slash; no additional URL validation is performed right now

Questions:

Are we OK with the path/URL detection, or do we want something with URL validation and errors for an invalid string?

Is it OK to just pass along the entire response with a HTTP>400 error? I'm not 100% clear if the request module is supposed to be agnostic about what it's requesting, or whether it's envisioned as only interfacing with the Hoodie API. In the latter case we could do the same error parsing as in https://github.com/hoodiehq/hoodie-client-account/blob/master/utils/request.js#L25-L30

gr2m commented 8 years ago

Thanks @andru, great work! Don’t worry about error parsing as we do it in the account module, it’s specific to the account module’s JSON API which we only use there right now. I’d keep hoodie.request more low level, returning the response body as error should be fine for now. I’m sure we run into problems later, but we can fix it one problem at a time :)

gr2m commented 8 years ago

Besides the error handling with statusCode >= 400 it looks good to me :+1:

andru commented 8 years ago

@gr2m great! The latest commit takes care of that. If it looks good, what should I do next? Rebase to a single commit? Any standards for commit messages?

gr2m commented 8 years ago

Looking great, thanks!!!!

If you could squash the commits, that’d be great. For this PR, please make 4 commits

for the README docs(README): ...
for package.json chore(package): ...
for tests test(request): initial version
for implementation feat(request): initial version

I started a PR to hoodie’s CONTRIBUTING.md#pull-request.

Does that make sense? Let me know if you have any questions, I’m happy to help with that

andru commented 8 years ago

That was epic! I've never done such a big rebase. I feel like a tiny god with the power to move code through time.

Assuming I've used my new powers for good and not evil, this should be good to go!

gr2m commented 8 years ago

Fantastic :clap: :bow: :fireworks: