Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Throw errors on !==200 status codes from RTS #662

Merged
merged 4 commits into from
Feb 1, 2017

Conversation

lukebarnard1
Copy link
Contributor

No description provided.

src/RtsClient.js Outdated
@@ -1,5 +1,25 @@
const q = require('q');
const request = q.nfbind(require('browser-request'));
const request = (opts) => {
const expectingJSONOnSucess = opts.json;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

success

src/RtsClient.js Outdated

return [response, body];
});
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'm not super keen on making request be something that is a wrapper around browser-request with more functionality, so I'd say if you're making it behave different on non 2xx responses, that should be a separate function. (Also why does it needs to explicitly parse the json? Doesn't passing the json flag through to request do that?)

We're gaining a bit of a collection of parts of react sdk doing requests in slightly different ways (DuckDuckGoProvider.js, MFileBody.js, DecryptFile.js and now here). We should try &make them roughly similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IRL: decision is to use fetch because there's no reason not to

return '?' + Object.keys(params).map((k) => {
return k + '=' + encodeURIComponent(params[k]);
}).join('&');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah, forgot fetch doesn't do this for you :/

@lukebarnard1 lukebarnard1 merged commit f8d7902 into develop Feb 1, 2017
@lukebarnard1 lukebarnard1 deleted the luke/rts-handle-errors branch February 1, 2017 11:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants