jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.39k forks source link

Trigger "changeId" need to be generate only if the content id change #4289

Open Noodlex opened 3 months ago

Noodlex commented 3 months ago

Affected component

The set function of backbone model

Expected behavior

Trigger "changeId" if the changeId is different of previous

Actual behavior

Always trigger "changeId"

Relevant documentation

No response

Software stack

Related issues, prior discussion and CCs

No response

Error

No response

Steps to reproduce

  1. fetch a backbone model
  2. re-fetch the same backbone model with the same id
  3. trigger "changeId" generate but there is no change id, it the same id

Additional information

No response

Suggested solution(s)

No response

Other remarks

No response

jgonggrijp commented 3 months ago

Thank you for reporting this. I was able to reproduce this with the following snippet of code:

var attrs = {id: 1, fruit: 'banana'};
var m = new Backbone.Model(attrs);
m.on('all', console.log);
m.set(attrs);
// Logs "changeId" to the console, plus event payload.

Before even trying that, I already took a peek at the code and immediately got a suspicion what might be causing this. The event is triggered here:

https://github.com/jashkenas/backbone/blob/4e642088c4810938a0510fa89456ef2619f446da/backbone.js#L523-L527

The condition of the if goes this.idAttribute in attrs, which checks whether the idAttribute is specified in the attributes passed to set. However, that does not take into account whether the specified value is actually different from the existing id. A more appropriate check would be _.contains(changes, this.idAttribute).

One other thing I noticed while looking at the code: changeId currently triggers even if the option silent: true is passed. Line 526 (from the snippet above) should probably be made conditional on !silent.

Noodlex commented 3 days ago

With the backbone-relation, the event change is often called.

I need to change my code to avoid this.

Is it possible to get a small version of this fix?

jgonggrijp commented 2 days ago

@Noodlex this ticket only refers to the changeId event, not change. I also don't think the fix can be any smaller than I described. That being said, everyone is welcome to submit a pull request!