length-property about ampersand-rest-collection HOT 6 CLOSED

I might be very off here, but

This is from the [email protected] dependency in [email protected]:

function Collection(models, options) {
    options || (options = {});
    if (options.model) this.model = options.model;
    if (options.comparator) this.comparator = options.comparator;
    if (options.parent) this.parent = options.parent;
    if (!this.mainIndex) {
        var idAttribute = this.model && this.model.prototype && this.model.prototype.idAttribute;
        this.mainIndex = idAttribute || 'id';
    }
    this._reset();
    this.initialize.apply(this, arguments);

    console.info(this.length, models, options)
    // => "0, Object {url: "/REST/event/orgs/1/events"}, Object {}"

    if (models) this.reset(models, assign({silent: true}, options));

    console.info(this.length, models, options)
    // => "1, Object {url: "/REST/event/orgs/1/events"}, Object {}
}

The models-param IS the options from

new HSEventCollection({
      url: url,
    });

and the options is an empty object!?

I have no idea how this actually works, but this does not look right to me.

Edit:
There seems to be other stuff in the collection-constructor as well. initialize should only accept an options argument, but it gets called with the constructor-arguments applied, so the models argument ends up being passed as the first argument. Should maybe be
this.initialize.call(this, options, models)
instead of
this.initialize.apply(this, arguments)

from ampersand-rest-collection.

When instantiating a new collection, the first argument is the models for the collection. url on the other hand is a property that goes in the collection.extend method.

var HSEventCollection = AmpersandCollection.extend({
  url: url
});

this.eventCollection = new HSEventCollection();
// or if you wanted to instantiate with initial models
this.eventCollection = new HSEventCollection([model1, model2, ...]);

from ampersand-rest-collection.

Hm, I see.

The docs say "new AmpersandCollection([models], [options])", so I assumed both to be independently optional (which they are not), and that would allow me to pass only an options-object to the "collection.initialize" function.

I'll have another look at it tomorrow.

Thanks.

from ampersand-rest-collection.

Ok, this is turning into another issue, but I'll just keep going a bit more before I close this, and open an issue on ampersand-model.

So, I read the collection docs as clearly saying that the constructor takes two optinal arguments

constructor/initialize new AmpersandCollection([models], [options])

-meaning that

var options = {
      url: url, // Or whatever...
}
var collection = new AmpersandCollection(options);

is perfectly legal. However, the code require the first argument to be present if you are passing the second argument, so the docs should in that case be:

constructor/initialize new AmpersandCollection([models, [options]])

I have never used Backbone, so if this is idiomatic there, I will just adopt to that convention. Otherwise, it I'd think this would make more sense:

function Collection(models, options) {

    // HERE!
    if (isObject(models) && options === undefined) {
        options = models;
        models = null;
    }

    options || (options = {});
    if (options.model) this.model = options.model;
    if (options.comparator) this.comparator = options.comparator;
    if (options.parent) this.parent = options.parent;
    if (!this.mainIndex) {
        var idAttribute = this.model && this.model.prototype && this.model.prototype.idAttribute;
        this.mainIndex = idAttribute || 'id';
    }
    this._reset();

    // AND HERE
    this.initialize.call(this, options, models);

    if (models) this.reset(models, assign({silent: true}, options));
}

-as all other initialize-methods takes the options as first parameter.

Thanks again.

from ampersand-rest-collection.

I believe this part was copied over from Backbone. From the Backbone collection constructor docs:

var spaces = new Backbone.Collection([], {
  model: Space
});

And passing options as the first parameter creates a collection with a length of 1. I'm not sure how idiomatic it is, but I know as a Backbone user for a few years before Ampersand the syntax of new Collection({url: url}) would be confusing to me 😄

I do agree that the docs are confusing. Would you be up for a PR to change that part?

from ampersand-rest-collection.

Yes, I'll do that.

from ampersand-rest-collection.