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 #41 - Added Api#fetchServerSettings. #55

Merged
merged 10 commits into from Jul 10, 2015
Merged

Conversation

n1k0
Copy link
Contributor

@n1k0 n1k0 commented Jul 9, 2015

Goals

The client should retrieve server settings from the /vX endpoint; for now, the only setting we need is the batch_limit one.

Tasks

  • On each outgoing HTTP request, first ensure we already have retrieved the settings information;
  • If we haven't yet, request that information from the /vX endpoint, then store the settings in a local Api property;
  • On subsequent requests, reuse the cached settings property to avoid hammering the server for no reason;
  • Write an integration test.
Notes

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 9, 2015

This is now ready for review. r=? @leplatrem @Natim @ametaireau

@@ -60,11 +61,11 @@ export default class Api {
* @return {String}
*/
endpoints(options={fullUrl: true}) {
var root = options.fullUrl ? this.remote : `/${this.version}`;
var root = options.fullUrl ? `${this.remote}/` : `/${this.version}/`;
var urls = {
root: () => root,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would have put the trailing slash here, so that assembling urls remains consistent (with / as join char)

@Natim
Copy link
Member

Natim commented Jul 10, 2015

Some version of the protocol don't have the settings parameter. What is the client behavior in case the settings is undefixed? Shall we consider that the server doesn't follow the protocol in that case or should we fallback to default values on client side?

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 10, 2015

Some version of the protocol don't have the settings parameter.

Which versions? The client is designed to support a single version of the protocol, which is v1. Do you mean it should also support previous versions of that version?

What is the client behavior in case the settings is undefi_n_ed?

You get an error. TBH I don't really want to start supporting versioning the version.

@Natim
Copy link
Member

Natim commented Jul 10, 2015

In that case it means we should document the list of provided settings in kinto documentation.

@leplatrem
Copy link
Contributor

r+

+1 to document settings explicitly in Kinto endpoints docs

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 10, 2015

+1 to document settings explicitly in Kinto endpoints docs

You mean in Kinto server, right? Server settings are transparent for client users.

n1k0 added a commit that referenced this pull request Jul 10, 2015
@n1k0 n1k0 merged commit de53e5c into master Jul 10, 2015
@n1k0 n1k0 deleted the fetch-server-settings branch July 10, 2015 10:34
@Natim
Copy link
Member

Natim commented Jul 13, 2015

You mean in Kinto server, right?

Yes, see Kinto/kinto#133

n1k0 added a commit that referenced this pull request Jul 13, 2015
@n1k0 n1k0 modified the milestone: 1.0.0 Jul 15, 2015
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.

None yet

3 participants