Skip to content

Commit

Permalink
remove addApiParams
Browse files Browse the repository at this point in the history
  • Loading branch information
Mathias Schreck committed Feb 7, 2014
1 parent 37e7ce6 commit 34e4338
Showing 1 changed file with 0 additions and 17 deletions.
17 changes: 0 additions & 17 deletions shared/syncer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ function serverSync(method, model, options) {
var api, data, urlParts, verb, req;

data = _.clone(options.data);
data = addApiParams(method, model, data);
options.url = this.getUrl(options.url, false, data);
verb = methodMap[method];
urlParts = options.url.split('?');
Expand Down Expand Up @@ -103,22 +102,6 @@ function serverSync(method, model, options) {
});
}

function addApiParams(method, model, params) {
params = params || {};
var ret = _.clone(params);

/**
* So, by default Backbone sends all of the model's
* attributes if we don't pass any in explicitly.
* This gets screwed up because we append the locale
* and currency, so let's replicate that behavior.
*/
if (model && _.isEqual(params, {}) && (method === 'create' || method === 'update')) {
_.extend(ret, model.toJSON());
}
return ret;
}

syncer.clientSync = clientSync;
syncer.sync = function sync() {
var syncMethod = isServer ? serverSync : clientSync;
Expand Down

8 comments on commit 34e4338

@naoak
Copy link

@naoak naoak commented on 34e4338 Feb 14, 2014

Choose a reason for hiding this comment

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

This change seems to prevent syncer from passing query parameters to API server.
How can I pass the parameters from controller fetching?

My model and controller are like the following.

// Ranking
var Base = require('./base');
module.exports = Base.extend({
  url: '/site/api/ranking',
  idAttribute: 'order'
});
module.exports.id = 'Ranking';

// HomeController
module.exports = {
  index: function(params, callback) {
    var spec = {
      view_ranking: {
        model: 'Ranking',
        params: {
          order: 'view'
        }
      },
      favorite_ranking: {
        model: 'Ranking',
        params: {
          order: 'favorite'
        }
      }
    };
    this.app.fetch(spec, function(error, result) {
      //
      // Seems to be not passing 'order' param
      //
      console.log(result.view_ranking);
      console.log(result.favorite_ranking);
      callback(error, result);
    });
  }
};

@c089
Copy link
Member

@c089 c089 commented on 34e4338 Feb 14, 2014

Choose a reason for hiding this comment

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

I believe this fixed #198, where parameters where sent even when you didn't want them too. To revert back to the old behaviour, I think you'll need to provide a url function in your model like I described there.

@naoak
Copy link

@naoak naoak commented on 34e4338 Feb 14, 2014

Choose a reason for hiding this comment

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

Thanks for your quick reply. It works!

After modified...

// Ranking
var Base = require('./base');
module.exports = Base.extend({
  // url: '/site/api/ranking',
  url: '/site/api/ranking?order=:order',
  idAttribute: 'order'
});
module.exports.id = 'Ranking';

@naoak
Copy link

@naoak naoak commented on 34e4338 Feb 14, 2014

Choose a reason for hiding this comment

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

The above fix takes effect on the first page rendering. But on ajax, the parameter is duplicated like the following.

/site/api/ranking?order=:view&order=:view

I think this will be fixed by #198, too. Do you have PR already, Chris?

@c089
Copy link
Member

@c089 c089 commented on 34e4338 Feb 14, 2014

Choose a reason for hiding this comment

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

Nope, nothing yet

@naoak
Copy link

@naoak naoak commented on 34e4338 Feb 15, 2014

Choose a reason for hiding this comment

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

A little bit disappointed to hear that... Thanks anyway :)

@jeromegn
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the initial complaint about #198 was a misunderstanding of how it works in rendr.

I believe it is intended behavior (and desired).

If you use the interpolatable URLs (with the :key syntax), then it won't append the unnecessary params.

I think this change should be reverted. It was very useful not to have to mess with that in every url in models and collections.

@naoak
Copy link

@naoak naoak commented on 34e4338 Feb 20, 2014

Choose a reason for hiding this comment

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

00_simple app can also reproduce the unnecessary params issue.

On server fetching,
"GET /repos/mojombo/grit HTTP/1.1 200"

On client fetching,
"GET /api/-/repos/mojombo/grit?owner=mojombo&name=grit HTTP/1.1 200"
"GET /api/travis-ci/-/repos/mojombo/grit?owner=mojombo&name=grit HTTP/1.1 200"

This issue was not occurred on 0.5.0-rc1.

Please sign in to comment.