rendrjs / rendr

Render your Backbone.js apps on the client and the server, using Node.js.
MIT License
4.09k stars 312 forks source link

Params from fetch spec are always appended to url when fetching data #198

Open c089 opened 10 years ago

c089 commented 10 years ago

Given a fetch spec like this

collection: {
  collection: 'Foo',
  params: { paramFromSpec: 'foo' }
}

And a collection with a custom url() method, e.g.

url: function() { return '/foo/?param1='+this.params.paramFromSpec+'&param2=bar'; }

When the syncer loads data, the resulting uri will always include all the params from url(), but also those from the spec, i.e.

/foo/?param1=foo&param2=bar&paramFromSpec=foo

This is due to the fact that the syncer sets options.data, which $.ajax then appends to the URL.

I think this is convenient because it allows to pass all the params from your spec as-they-are to the remote API, but if you want to modify parameters, it get's messy.

I'd gladly work on a PR for this but I'm not sure I understand the intention of this and wanted to be sure not to run into the wrong direction.

jeromegn commented 10 years ago

I'm not sure if it's documented, but rendr interpolates the urls for models and collections.

url: "/foo/?param1=:paramFromSpec&param2=bar"

Should work. Also, they won't be added to the query param if they've been interpolated.

spikebrehm commented 10 years ago

I agree with @jeromegn that the interpolation will fix this particular case. It does seem like a useful default to have the fetch params automatically be appended to the URL. Maybe the best approach would be to keep this default behavior but add a boolean configuration property on models & collections like this.appendParams.

c089 commented 10 years ago

It seems useful, but is also potentially unwanted - how about having a method in the base class so if you want the behaviour you can opt-in? Something similar to

url: function () { return '/foo/bar?customParam=a&'  + this.encodedParams(); }

would be less surprising. Explicit > implicit.

c089 commented 10 years ago

Also IIRC this won't be caught by unit tests for the model because they're appended later - which is why I was suprised when I spotted the extra params in the logs.

ghost commented 10 years ago

c089, can you publish your PR please ? That bug destroyed my app :(