Skip to content
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

Merged
merged 1 commit into from
Aug 7, 2015
Merged

[Fixes #108] Refactor CRUD interface #110

merged 1 commit into from
Aug 7, 2015

Conversation

Vijar
Copy link
Contributor

@Vijar Vijar commented Jul 13, 2015

@redonkulus @mridgway @lingyan

Sorry this is so huge, one thing led to another down the rabbit hole. This PR includes:

  • New interface for CRUD methods w/ backwards compatibility
    • Deprecated old interface
  • Renamed all instances of 'fetchers' to 'services' w/ backwards compatibility
    • Deprecated registerFetcher and getFetcher
  • Fixed an issue where custom constructGetUri function wasn't falling back properly
  • Housekeeping on test suite
  • Deleted old batching code

```js
var Fetcher = require('fetchr');
var fetcher = new Fetcher({
xhrPath: '/myCustomAPIEndpoint'
Copy link
Contributor

Choose a reason for hiding this comment

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

xhrTimeout got lost here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, fixing.

@Vijar Vijar mentioned this pull request Jul 16, 2015
@@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

what was name removed?

Copy link
Contributor Author

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.

@Vijar
Copy link
Contributor Author

Vijar commented Jul 30, 2015

@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
Copy link
Contributor

Choose a reason for hiding this comment

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

add @chainable

@redonkulus
Copy link
Contributor

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.

@Vijar
Copy link
Contributor Author

Vijar commented Aug 4, 2015

@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?

@mridgway
Copy link
Collaborator

mridgway commented Aug 5, 2015

lgtm. This should be a new minor version since it introduces new warnings for deprecated APIs.

@redonkulus
Copy link
Contributor

👍

Vijar added a commit that referenced this pull request Aug 7, 2015
[Fixes #108] Refactor CRUD interface
@Vijar Vijar merged commit d786b20 into master Aug 7, 2015
@Vijar Vijar removed the in progress label Aug 7, 2015
@Vijar Vijar deleted the interface branch August 7, 2015 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants