-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fixes #108] Refactor CRUD interface #110
Conversation
```js | ||
var Fetcher = require('fetchr'); | ||
var fetcher = new Fetcher({ | ||
xhrPath: '/myCustomAPIEndpoint' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xhrTimeout got lost here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, fixing.
@@ -41,7 +29,7 @@ function parseResponse(response) { | |||
try { | |||
return JSON.parse(response.responseText); | |||
} catch (e) { | |||
debug('json parse failed:' + e, 'error', NAME); | |||
debug('json parse failed:' + e, 'error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was name removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just the string FetcherClient
but debug
automatically annotates so it was unnecessary.
@redonkulus updated. |
* Add params to this fetcher request | ||
* @method params | ||
* @memberof Request | ||
* @param {Object} params Information carried in query and matrix parameters in typical REST API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add @chainable
Looks good, just come additional minor docblock comments. I'd like to see this applied internally to ensure we did not introduce any breaking changes before we publish. |
@redonkulus I tested this internally, and found no breaking changes. @mridgway can you give this a once over just so we have more than one person reviewing this PR? |
lgtm. This should be a new minor version since it introduces new warnings for deprecated APIs. |
👍 |
[Fixes #108] Refactor CRUD interface
@redonkulus @mridgway @lingyan
Sorry this is so huge, one thing led to another down the rabbit hole. This PR includes:
registerFetcher
andgetFetcher
constructGetUri
function wasn't falling back properly