backbone-paginator/backbone.paginator

fetch overwrites options.data with current state

Closed this issue · 1 comments

dan-f commented

The current implementation of fetch allows the caller to pass in an options.data object which maps querystring parameter names to values to be sent to the server. This options.data is merged together with the collection's current state by examining the state and queryParams objects.

The problem is that state/queryParams currently take precedence. For example, if I run:

collection.fetch({data: {page: 2});

The collection will still fetch the first page. This seems like a bug to me; I expected options.data to take precedence.

Here is the problematic code:

      var i, kvp, k, v, kvps = _pairs(queryParams), thisCopy = _clone(this);
      for (i = 0; i < kvps.length; i++) {
        kvp = kvps[i], k = kvp[0], v = kvp[1];
        v = _isFunction(v) ? v.call(thisCopy) : v;
        if (state[k] != null && v != null) {
          data[v] = state[k];
        }
      }

And a fix:

      var i, kvp, k, v, kvps = _pairs(queryParams), thisCopy = _clone(this);
      for (i = 0; i < kvps.length; i++) {
        kvp = kvps[i], k = kvp[0], v = kvp[1];
        v = _isFunction(v) ? v.call(thisCopy) : v;
        if (state[k] != null && v != null && _.isUndefined(data[v])) {  // Only set if this key/val pair isn't already present in `data`.
          data[v] = state[k];
        }
      }

...And a more concise fix:

      _.each(queryParams, function (v, k) {
        v = _isFunction(v) ? v.call(thisCopy) : v;
        if (state[k] != null && v != null && _.isUndefined(data[v])) {
          data[v] = state[k];
        }
      }, this);

This looks good. Please submit a PR along with a test. Thanks so much.