From 0a18362b70baddf80017c2104c9815fce80ec175 Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Wed, 19 Jul 2023 00:18:27 +0200 Subject: [PATCH 1/8] Add a test that reproduces #4262 literally --- test/collection.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/collection.js b/test/collection.js index d0890f81f..4ab77943b 100644 --- a/test/collection.js +++ b/test/collection.js @@ -654,6 +654,15 @@ assert.equal(collection.length, 1); }); + QUnit.test('failing create with wait:true triggers error event (#4262)', function(assert) { + assert.expect(1); + var collection = new Backbone.Collection; + collection.url = '/test'; + collection.on('error', function() { assert.ok(true); }); + collection.create({id: '1'}, {wait: true}); + this.ajaxSettings.error(); + }); + QUnit.test('initialize', function(assert) { assert.expect(1); var Collection = Backbone.Collection.extend({ From 0b831f19af64ca4c992567a2fb1c461568fcdfdf Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Wed, 19 Jul 2023 00:26:38 +0200 Subject: [PATCH 2/8] Also add a test to verify that the problem is not in Model.save (#4262) --- test/model.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/model.js b/test/model.js index b3092df7f..d5e20080a 100644 --- a/test/model.js +++ b/test/model.js @@ -712,6 +712,15 @@ this.ajaxSettings.success(); }); + QUnit.test('failing save with wait:true triggers error event (#4262)', function(assert) { + assert.expect(1); + var model = new Backbone.Model; + model.urlRoot = '/test'; + model.on('error', function() { assert.ok(true); }); + model.save({id: '1'}, {wait: true}); + this.ajaxSettings.error(); + }); + QUnit.test('fetch', function(assert) { assert.expect(2); doc.fetch(); From 2104aa771fae7a258dbb091b50f47ba85d6de456 Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Wed, 19 Jul 2023 00:49:28 +0200 Subject: [PATCH 3/8] But does the created model trigger 'error'? (#4262) Yes, it does. --- test/collection.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/collection.js b/test/collection.js index 4ab77943b..a8d55fcd9 100644 --- a/test/collection.js +++ b/test/collection.js @@ -655,11 +655,12 @@ }); QUnit.test('failing create with wait:true triggers error event (#4262)', function(assert) { - assert.expect(1); + assert.expect(2); var collection = new Backbone.Collection; collection.url = '/test'; collection.on('error', function() { assert.ok(true); }); - collection.create({id: '1'}, {wait: true}); + var model = collection.create({id: '1'}, {wait: true}); + model.on('error', function() { assert.ok(true); }); this.ajaxSettings.error(); }); From b22474b5179e575f98de3c69386fcfaba1536caf Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Wed, 19 Jul 2023 00:53:02 +0200 Subject: [PATCH 4/8] Wait, how about success? (#4262) Amazingly, this test passes. --- test/collection.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/collection.js b/test/collection.js index a8d55fcd9..5486b99c4 100644 --- a/test/collection.js +++ b/test/collection.js @@ -664,6 +664,16 @@ this.ajaxSettings.error(); }); + QUnit.test('successful create with wait:true triggers success event (#4262)', function(assert) { + assert.expect(2); + var collection = new Backbone.Collection; + collection.url = '/test'; + collection.on('sync', function() { assert.ok(true); }); + var model = collection.create({id: '1'}, {wait: true}); + model.on('sync', function() { assert.ok(true); }); + this.ajaxSettings.success(); + }); + QUnit.test('initialize', function(assert) { assert.expect(1); var Collection = Backbone.Collection.extend({ From 7c04484843d28fe758663b20bc36da3a238ffc06 Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Wed, 19 Jul 2023 01:24:27 +0200 Subject: [PATCH 5/8] Fix the problem (#4262) --- backbone.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/backbone.js b/backbone.js index 251d6c5fa..aaa9f759a 100644 --- a/backbone.js +++ b/backbone.js @@ -1079,10 +1079,26 @@ if (!wait) this.add(model, options); var collection = this; var success = options.success; + var forwardPristineError; options.success = function(m, resp, callbackOpts) { - if (wait) collection.add(m, callbackOpts); + if (wait) { + model.off('error', forwardPristineError); + collection.add(m, callbackOpts); + } if (success) success.call(callbackOpts.context, m, resp, callbackOpts); }; + // In case of wait:true, our collection is not listening to any + // of the model's events yet, so it will not forward the error + // event. In this special case, we need to listen for it + // separately and handle the event just once. + // (The reason we don't need to do this for the sync event is + // in the success handler above: we add the model first, which + // causes the collection to listen, and then invoke the callback + // that triggers the event.) + if (wait) { + forwardPristineError = _.bind(this._onModelEvent, this, 'error'); + model.once('error', forwardPristineError); + } model.save(null, options); return model; }, From f30f9741981c472e1e2a482c81ca6bda3ba0cfd3 Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Thu, 20 Jul 2023 22:37:26 +0200 Subject: [PATCH 6/8] Add sanity checks against double forward and error listener erasion (#4262) --- test/collection.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/collection.js b/test/collection.js index 5486b99c4..adf16dfe9 100644 --- a/test/collection.js +++ b/test/collection.js @@ -674,6 +674,27 @@ this.ajaxSettings.success(); }); + QUnit.test('failing create pre-existing with wait:true triggers once (#4262)', function(assert) { + assert.expect(1); + var model = new Backbone.Model; + var collection = new Backbone.Collection([model]); + collection.url = '/test'; + collection.on('error', function() { assert.ok(true); }); + collection.create(model, {wait: true}); + this.ajaxSettings.error(); + }); + + QUnit.test('successful create pre-existing with wait:true preserves other error bindings (#4262)', function(assert) { + assert.expect(1); + var model = new Backbone.Model; + var collection = new Backbone.Collection([model]); + collection.url = '/test'; + model.on('error', function() { assert.ok(true); }); + collection.create(model, {wait: true}); + this.ajaxSettings.success(); + model.trigger('error'); + }); + QUnit.test('initialize', function(assert) { assert.expect(1); var Collection = Backbone.Collection.extend({ From c87833877f4b14484c951cb688765f992f36f6eb Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Thu, 20 Jul 2023 22:39:16 +0200 Subject: [PATCH 7/8] Apply fix suggested by @paulfalgout (#4262) See https://github.com/jashkenas/backbone/pull/4265#discussion_r1269749470 --- backbone.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/backbone.js b/backbone.js index aaa9f759a..f1c98d087 100644 --- a/backbone.js +++ b/backbone.js @@ -1082,7 +1082,7 @@ var forwardPristineError; options.success = function(m, resp, callbackOpts) { if (wait) { - model.off('error', forwardPristineError); + m.off('error', forwardPristineError, this); collection.add(m, callbackOpts); } if (success) success.call(callbackOpts.context, m, resp, callbackOpts); @@ -1096,8 +1096,11 @@ // causes the collection to listen, and then invoke the callback // that triggers the event.) if (wait) { - forwardPristineError = _.bind(this._onModelEvent, this, 'error'); - model.once('error', forwardPristineError); + forwardPristineError = function(mod, col, opt) { + if (this.has(mod)) return; + this._onModelEvent('error', mod, col, opt); + }; + model.once('error', forwardPristineError, this); } model.save(null, options); return model; From c8305eef198e0e39eda6743b37c76327468529d3 Mon Sep 17 00:00:00 2001 From: Julian Gonggrijp Date: Thu, 20 Jul 2023 22:52:08 +0200 Subject: [PATCH 8/8] Deforest and document (#4262) --- backbone.js | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/backbone.js b/backbone.js index f1c98d087..d8f26a737 100644 --- a/backbone.js +++ b/backbone.js @@ -1079,10 +1079,9 @@ if (!wait) this.add(model, options); var collection = this; var success = options.success; - var forwardPristineError; options.success = function(m, resp, callbackOpts) { if (wait) { - m.off('error', forwardPristineError, this); + m.off('error', this._forwardPristineError, this); collection.add(m, callbackOpts); } if (success) success.call(callbackOpts.context, m, resp, callbackOpts); @@ -1096,11 +1095,7 @@ // causes the collection to listen, and then invoke the callback // that triggers the event.) if (wait) { - forwardPristineError = function(mod, col, opt) { - if (this.has(mod)) return; - this._onModelEvent('error', mod, col, opt); - }; - model.once('error', forwardPristineError, this); + model.once('error', this._forwardPristineError, this); } model.save(null, options); return model; @@ -1239,8 +1234,19 @@ } } this.trigger.apply(this, arguments); - } + }, + // Internal callback method used in `create`. It serves as a + // stand-in for the `_onModelEvent` method, which is not yet bound + // during the `wait` period of the `create` call. We still want to + // forward any `'error'` event at the end of the `wait` period, + // hence a customized callback. + _forwardPristineError: function(model, collection, options) { + // Prevent double forward if the model was already in the + // collection before the call to `create`. + if (this.has(model)) return; + this._onModelEvent('error', model, collection, options); + } }); // Defining an @@iterator method implements JavaScript's Iterable protocol.