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

[Noodlex]

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

• Backbone 1.6.0

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]

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:

backbone/backbone.js

Lines 523 to 527 in 4e64208

	if (this.idAttribute in attrs) {
	var prevId = this.id;
	this.id = this.get(this.idAttribute);
	this.trigger('changeId', this, prevId, options);
	}

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]

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]

@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!