Bug Collection:reset calls add model in case of models undefined

[xQwexx]

Currently if you call reset() on a collection it will use the add(models) function with models == undefined.

So I think it is rather logical to call the add if the models is not undefined.

[jgonggrijp]

Thank you for your willingness to contribute, @xQwexx.

I think the change you are proposing does not actually make a difference. As you said, reset passes its models to add. add in turn passes them to set:

https://github.com/jashkenas/backbone/blob/0d455df4e15d814ed9c3f24be1ab4ea23c5c630d/backbone.js#L818

set, in turn, checks whether models is defined and returns immediately otherwise:

https://github.com/jashkenas/backbone/blob/0d455df4e15d814ed9c3f24be1ab4ea23c5c630d/backbone.js#L839

A general convention in Backbone is that when an empty array is allowed, null or undefined is also allowed.

Next time when you propose a change, please discuss not only why you think the change makes sense, but also why you think it is necessary. The necessity should generally be proven with a test that fails without the change and passes with the change.

I will close this now, but please feel free to reply if you still have questions or comments. We can reopen the PR if needed.

[xQwexx]

Hi @jgonggrijp 😄
Sob to add some more information I am using typescript, so the typed version of backbone, where the add syntax's

        add(model: {} | TModel, options?: AddOptions): TModel;
        add(models: Array<{} | TModel>, options?: AddOptions): TModel[];

But when I called the reset() on my collection class which extends from the Backbone.Collections and also overriding the add functionality.

  add(model: Array<Record<string, any>> | TModel, options?: AddOptions): TModel;
  add(models: Array<Array<Record<string, any>> | TModel>, options?: AddOptions): TModel[];
  add(model: unknown, options?: AddOptions): any {
    //Note: the undefined case needed because backbonejs not handle the reset() correctly
    var models = isArray(model) ? model : [model];
    models = models.map(m => (m instanceof this.newModel ? m : new this.newModel(this.module, m)));
    return super.add(isArray(model) ? models : models[0], options);
  }

I got an exception because I didn't count on that this add function could be called with undefined parameter, because of the type description. So the cause of the problem that the backbone framework calls the add function with invalid parameters (Which also not do anything, I think it is a better solution to validate beforehand) So I think there could be two solution two fix this one is to update the type description to accept undefined which I personally don't like, because I don't make sense to add undefined into a collection. Or two make sure that the framework itself uses the public API, which I think better (its easier to use the framework and I don't need to handle this scenario).

If something not clear, I'm happy to try explain in more details Thank you for the fast response 😄

[jgonggrijp]

(...) I am using typescript, so the typed version of backbone,

Just to be clear, there is no "typed version of backbone". I presume you mean @types/backbone. That is just an unofficial package by volunteers who wanted to make TypeScript work with Backbone. I contributed to it, too, but that still doesn't make it official. Backbone (the JavaScript-based repository and package that we are currently discussing) holds the final truth about how Backbone should behave.

where the add syntax's

        add(model: {} | TModel, options?: AddOptions): TModel;
        add(models: Array<{} | TModel>, options?: AddOptions): TModel[];

That typing is wrong. You are allowed to omit the first argument as well, so it should be as below.

        add(model?: {} | TModel, options?: AddOptions): TModel;
        add(models?: Array<{} | TModel>, options?: AddOptions): TModel[];

I suggest you submit this as a pull request to DefinitelyTyped instead. I'll be happy to review and approve your patch over there. Be sure to extend the tests, though.

  add(model: Array<Record<string, any>> | TModel, options?: AddOptions): TModel;
  add(models: Array<Array<Record<string, any>> | TModel>, options?: AddOptions): TModel[];

If the code quoted below is your whole override of add, then I think you do not need to override add in the first place, since you are not actually changing its behavior. It looks very much like you are just working around typing issues. That said, I just wanted to point out that you seem to have too many Arrays in the above signatures. I think you intended model: Record<string, any> | TModel and models: Array<Record<string, any> | TModel>, respectively. Besides the fact that they should be optional, of course.

  add(model: unknown, options?: AddOptions): any {
    //Note: the undefined case needed because backbonejs not handle the reset() correctly
    var models = isArray(model) ? model : [model];
    models = models.map(m => (m instanceof this.newModel ? m : new this.newModel(this.module, m)));
    return super.add(isArray(model) ? models : models[0], options);
  }

I got an exception because I didn't count on that this add function could be called with undefined parameter, because of the type description. So the cause of the problem that the backbone framework calls the add function with invalid parameters

No, the cause of the problem is that the typings are wrong.

(Which also not do anything, I think it is a better solution to validate beforehand) So I think there could be two solution two fix this one is to update the type description to accept undefined

Yes. This is the current semantic of Backbone, so @types/backbone should reflect that.

which I personally don't like, because I don't make sense to add undefined into a collection.

Like it or not, Backbone has allowed and handled this since its inception in 2009. A lot of code would break if we were to suddenly stop allowing it.

Or two make sure that the framework itself uses the public API,

The framework is the public API. @types/backbone does not dictate how Backbone should behave. Rather, the job of @types/backbone is to truthfully reflect what is in Backbone.

which I think better (its easier to use the framework and I don't need to handle this scenario).

It is easier still to just make the typings more accurate, for everyone involved.

(...) Thank you for the fast response 😄

Welcome!