Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2: Can of Promises #3729

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

v2: Can of Promises #3729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bfa268c
sync returns a Promise
au-phiware Jul 21, 2015
e9f019e
Play nice with callbacks.
au-phiware Jul 21, 2015
ecb606f
Any constructor or may provide a Promise.
au-phiware Jul 22, 2015
91227ee
Merge branch 'master' of https://github.com/jashkenas/backbone
au-phiware Jul 22, 2015
9adf677
Router#execute may provide a Promise or trigger an error.
au-phiware Jul 22, 2015
5b39cf9
Permit routers to store metadata in History's handlers.
au-phiware Jul 22, 2015
bdaa11b
Merge branch 'master' of https://github.com/jashkenas/backbone
au-phiware Jul 24, 2015
ca27d88
Avoid conflict with potential reserved word.
au-phiware Jul 26, 2015
53e9555
Don't assume `Promise` is a global.
au-phiware Jul 26, 2015
0d4fe5b
Relax the definition of a `Promise`.
au-phiware Jul 26, 2015
0bee4c0
Reject with an error when the error is known.
au-phiware Jul 26, 2015
403bc44
Event listener errors are handled by the caller of save, fetch, destroy.
au-phiware Jul 26, 2015
fb45026
Fix typo
au-phiware Jul 26, 2015
820dd25
Depend on promise.js.
au-phiware Jul 26, 2015
509adab
Don't prescribe Promise implementation and fallback to global.
au-phiware Jul 27, 2015
747ffaa
Clarify the extent of the use of Promises.
au-phiware Jul 27, 2015
7eddccd
Revert "Any constructor or may provide a Promise."
au-phiware Jul 27, 2015
2e8c79c
Remove my 'mission statement'.
au-phiware Jul 27, 2015
0833ee7
Model#save rejects with validationError that is created by Model#set.
au-phiware Jul 27, 2015
44bec7b
Omit Promise from UMD wrapper.
au-phiware Jul 27, 2015
667f81d
Return callback result for correct event propagation.
au-phiware Jul 27, 2015
21ab2ee
Merge branch 'master' of https://github.com/jashkenas/backbone
au-phiware Aug 8, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
226 changes: 146 additions & 80 deletions backbone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@
// Proxy `Backbone.sync` by default -- but override this if you need
// custom syncing semantics for *this* particular model.
sync: function() {
return Backbone.sync.apply(this, arguments);
return Promise.resolve(Backbone.sync.apply(this, arguments));
},

// Get the value of an attribute.
Expand Down Expand Up @@ -587,15 +587,21 @@
fetch: function(options) {
options = _.extend({parse: true}, options);
var model = this;
var success = options.success;
options.success = function(resp) {
var serverAttrs = options.parse ? model.parse(resp, options) : resp;
if (!model.set(serverAttrs, options)) return false;
if (success) success.call(options.context, model, resp, options);
model.trigger('sync', model, resp, options);
};
wrapError(this, options);
return this.sync('read', this, options);
var success = options.success || _.identity;
var error = options.error;
delete options.success;
delete options.error;
return wrapError(this.sync('read', this, options), this, options, error)
.then(function(resp) {
var serverAttrs = options.parse ? model.parse(resp, options) : resp;
if (model.set(serverAttrs, options))
return Promise.resolve(success.call(options.context, model, resp, options))
.then(function(delivered) {
model.trigger('sync', model, resp, options);
return delivered;
});
return model;
});
},

// Set a hash of model attributes, and sync the model to the server.
Expand All @@ -613,43 +619,48 @@

options = _.extend({validate: true, parse: true}, options);
var wait = options.wait;
var success = options.success || _.identity;
var error = options.error;
delete options.success;
delete options.error;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't modify the options object

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err, but the master branch does modify the options object already... I guess you object to the delete lines?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, but I'm not modifying the object that is provided in the arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you care to delete these properties? It shouldn't matter if they stick around, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want them called by ajax (i.e. jQuery or whatever it might be), because I want them to be apart of the promise chain.


// If we're not waiting and attributes exist, save acts as
// `set(attr).save(null, opts)` with validation. Otherwise, check if
// the model will be valid when the attributes, if any, are set.
if (attrs && !wait) {
if (!this.set(attrs, options)) return false;
if (!this.set(attrs, options)) return Promise.reject(this.validationError);
} else {
if (!this._validate(attrs, options)) return false;
if (!this._validate(attrs, options)) return Promise.reject(this.validationError);
}

// After a successful server-side save, the client is (optionally)
// updated with the server-side state.
var model = this;
var success = options.success;
var attributes = this.attributes;
options.success = function(resp) {
// Ensure attributes are restored during synchronous saves.
model.attributes = attributes;
var serverAttrs = options.parse ? model.parse(resp, options) : resp;
if (wait) serverAttrs = _.extend({}, attrs, serverAttrs);
if (serverAttrs && !model.set(serverAttrs, options)) return false;
if (success) success.call(options.context, model, resp, options);
model.trigger('sync', model, resp, options);
};
wrapError(this, options);

// Set temporary attributes if `{wait: true}` to properly find new ids.
if (attrs && wait) this.attributes = _.extend({}, attributes, attrs);

var method = this.isNew() ? 'create' : (options.patch ? 'patch' : 'update');
if (method === 'patch' && !options.attrs) options.attrs = attrs;
var xhr = this.sync(method, this, options);
var promise = wrapError(this.sync(method, this, options), this, options, error);

// Restore attributes.
this.attributes = attributes;

return xhr;
return promise.then(function(resp) {
// Ensure attributes are restored during synchronous saves.
model.attributes = attributes;
var serverAttrs = options.parse ? model.parse(resp, options) : resp;
if (wait) serverAttrs = _.extend({}, attrs, serverAttrs);
if (!serverAttrs || model.set(serverAttrs, options))
return Promise.resolve(success.call(options.context, model, resp, options))
.then(function(delivered) {
model.trigger('sync', model, resp, options);
return delivered;
});
return model;
});
},

// Destroy this model on the server if it was already persisted.
Expand All @@ -658,29 +669,33 @@
destroy: function(options) {
options = options ? _.clone(options) : {};
var model = this;
var success = options.success;
var wait = options.wait;
var success = options.success;
var error = options.error;
delete options.success;
delete options.error;

var destroy = function() {
model.stopListening();
model.trigger('destroy', model, model.collection, options);
};

options.success = function(resp) {
if (wait) destroy();
if (success) success.call(options.context, model, resp, options);
if (!model.isNew()) model.trigger('sync', model, resp, options);
};

var xhr = false;
if (this.isNew()) {
_.defer(options.success);
} else {
wrapError(this, options);
xhr = this.sync('delete', this, options);
}
var isNew = model.isNew();
var promise = isNew ? Promise.resolve(model.attributes) :
wrapError(this.sync('delete', this, options), this, options, error);
if (!wait) destroy();
return xhr;
return promise.then(function(resp) {
var promise = Promise.resolve(model);
if (wait) destroy();
if (success)
promise = Promise.resolve(success.call(options.context, model, resp, options));
if (!isNew)
promise = promise.then(function(delivered) {
model.trigger('sync', model, resp, options);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does promise need to be redefined for this (I mean why not let a race (of sorts, the sync one will always win in all implementations I've seen) happen between sync and the returned promise resolve

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be written differently, but the point is that when an existing model is destroyed then the sync event will be triggered after the success callback. Providing a hook to the developer to resolve any rejection by the sync method before the event is triggered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I got confused, the error callback provides the hook to resolve a rejected promise from sync (not the success callback). The success callback provides a hook to change the object (by reference) with which the returned Promise is fulfilled (and/or modify the model, if it wants) before the sync event is triggered. Event#trigger doesn't return anything particularly useful to the chain of promises so this is the only chance that the destroy method can participant in the promise chain after the call to sync and before sync is triggered. (Same goes for the save method.)

Thinking about it some more, you could argue that the sync event doesn't care how or what the promise is fulfilled (i.e. delivered is not passed to Events#trigger), so what does it matter...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that's entirely your point isn't it? Sorry, I'm a bit slow...

So, to that point: the triggering of sync must be present in the returned chained of promises so that any errors can be caught by the caller.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super important to me, the other day I got bit by an error that was being thrown by a jquery plugin inside a stickit method that is called by a sync trigger. The error was swallowed deep inside a promise (of my own making) and was very hard to trace. If I had this code the error object would have been propagated up to the user and the user would have probably/hopefully been presented with an error instead of half a page of broken content. Of course the outcome is that I had to work around the buggy jquery plugin (there's no escape from that!) but there would have been a better developer experience at the very least.

So, I wrote a test: 403bc44

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was we can avoid returning a new promise and instead just return

if (!isNew) {
          promise.then(function(delivered) {
            model.trigger('sync', model, resp, options);
         });
}

Note not redefining promise = promise.then

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resulting promise of that expression needs to be returned.

return delivered;
});
return promise;
});
},

// Default URL for the model's representation on the server -- if you're
Expand All @@ -693,7 +708,12 @@
urlError();
if (this.isNew()) return base;
var id = this.get(this.idAttribute);
return base.replace(/[^\/]$/, '$&/') + encodeURIComponent(id);
var consume = function(base) {
return base.replace(/[^\/]$/, '$&/') + encodeURIComponent(id);
};
if (isPromise(base))
return base.then(consume);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to make a helper for this. Also instanceof is not the correct way to check for Promises as they should be implementation independent

See https://github.com/then/is-promise

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I used instanceof because I'm assuming ES6 Promises.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 0d4fe5b what you had in mind?

Also, shall go ahead and drop the polyfill under test/vendor?

return consume(base);
},

// **parse** converts a response into the hash of attributes to be `set` on
Expand Down Expand Up @@ -783,7 +803,7 @@

// Proxy `Backbone.sync` by default.
sync: function() {
return Backbone.sync.apply(this, arguments);
return Promise.resolve(Backbone.sync.apply(this, arguments));
},

// Add a model, or list of models to the set. `models` may be Backbone
Expand Down Expand Up @@ -1004,16 +1024,21 @@
// data will be passed through the `reset` method instead of `set`.
fetch: function(options) {
options = _.extend({parse: true}, options);
var success = options.success;
var collection = this;
options.success = function(resp) {
var method = options.reset ? 'reset' : 'set';
collection[method](resp, options);
if (success) success.call(options.context, collection, resp, options);
collection.trigger('sync', collection, resp, options);
};
wrapError(this, options);
return this.sync('read', this, options);
var success = options.success || _.identity;
var error = options.error;
delete options.success;
delete options.error;
return wrapError(this.sync('read', this, options), this, options, error)
.then(function(resp) {
var method = options.reset ? 'reset' : 'set';
collection[method](resp, options);
return Promise.resolve(success.call(options.context, collection, resp, options))
.then(function(delivered) {
collection.trigger('sync', collection, resp, options);
return delivered;
});
});
},

// Create a new instance of a model in this collection. Add the model to the
Expand All @@ -1026,13 +1051,18 @@
if (!model) return false;
if (!wait) this.add(model, options);
var collection = this;
var success = options.success;
options.success = function(model, resp, callbackOpts) {
if (wait) collection.add(model, callbackOpts);
if (success) success.call(callbackOpts.context, model, resp, callbackOpts);
var success = options.success || _.identity;
var error = options.error;
delete options.error;
options.success = function(_, resp, opts) {
options = opts;
return resp;
};
model.save(null, options);
return model;
return model.save(null, options)
.then(function(resp) {
if (wait) collection.add(model, options);
return Promise.resolve(success.call(options.context, model, resp, options));
});
},

// **parse** converts a response into a list of models to be added to the
Expand Down Expand Up @@ -1351,9 +1381,7 @@
var params = {type: type, dataType: 'json'};

// Ensure that we have a URL.
if (!options.url) {
params.url = _.result(model, 'url') || urlError();
}
params.url = options.url || _.result(model, 'url') || urlError();

// Ensure that we have the appropriate request data.
if (options.data == null && model && (method === 'create' || method === 'update' || method === 'patch')) {
Expand Down Expand Up @@ -1385,17 +1413,21 @@
}

// Pass along `textStatus` and `errorThrown` from jQuery.
var error = options.error;
options.error = function(xhr, textStatus, errorThrown) {
options.textStatus = textStatus;
options.errorThrown = errorThrown;
if (error) error.call(options.context, xhr, textStatus, errorThrown);
};

// Make the request, allowing the user to override any Ajax options.
var xhr = options.xhr = Backbone.ajax(_.extend(params, options));
model.trigger('request', model, xhr, options);
return xhr;
var consume = function(url) {
params.url = url || urlError();
var promise = Promise.resolve(options.xhr = Backbone.ajax(_.extend(params, _.omit(options, 'url'))));
model.trigger('request', model, promise, options);
return promise;
};
if (isPromise(params.url))
return params.url.then(consume);
return consume(params.url);
};

// Map from CRUD to HTTP for our default `Backbone.sync` implementation.
Expand Down Expand Up @@ -1455,19 +1487,42 @@
var router = this;
Backbone.history.route(route, function(fragment) {
var args = router._extractParameters(route, fragment);
if (router.execute(callback, args, name) !== false) {
router.trigger.apply(router, ['route:' + name].concat(args));
router.trigger('route', name, args);
Backbone.history.trigger('route', router, name, args);
}
var result = router.execute(callback, args, name);
var consume = function(result) {
if (result !== false) {
router.trigger.apply(router, ['route:' + name].concat(args));
router.trigger('route', name, args);
Backbone.history.trigger('route', router, name, args);
}
};
if (isPromise(result))
result.then(consume);
else
consume(result);
});
return this;
},

// Execute a route handler with the provided parameters. This is an
// excellent place to do pre-route setup or post-route cleanup.
execute: function(callback, args, name) {
if (callback) callback.apply(this, args);
var router = this;
var error = function(err) {
router.trigger('error', err, name, args, callback);
};
if (callback) {
try {
var route = callback.apply(this, args);
if (isPromise(route)) {
return route['catch'](error);
}
return route;
} catch(e) {
error(e);
}
} else {
error();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to add additional tests for this functionality.

Fwiw (although it may be irrelevant to this PR), if all of my desired changes to the router land then this method will no longer exist.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about jmeas/backbone.base-router? I should like to check it out :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, should have linked the PR. I was referring to #3660. That PR is heavily inspired by the ideas in BaseRouter, though :)

},

// Simple proxy to `Backbone.history` to save a fragment into the history.
Expand Down Expand Up @@ -1709,8 +1764,8 @@

// Add a route to be tested when the fragment changes. Routes added later
// may override previous routes.
route: function(route, callback) {
this.handlers.unshift({route: route, callback: callback});
route: function(route, callback, meta) {
this.handlers.unshift(_.defaults({route: route, callback: callback}, meta));
},

// Checks the current URL to see if it has changed, and if it has,
Expand Down Expand Up @@ -1865,15 +1920,26 @@
throw new Error('A "url" property or function must be specified');
};

// Wrap an optional error callback with a fallback error event.
var wrapError = function(model, options) {
var error = options.error;
options.error = function(resp) {
if (error) error.call(options.context, model, resp, options);
model.trigger('error', model, resp, options);
};
// Allow a callback to resolve a rejected promise, otherwise promise
// will remain rejected and an `error` event will be triggered on model.
var wrapError = function(promise, model, options, callback) {
return Promise.resolve(promise)
['catch'](function(resp) {
if (callback)
return callback.call(options.context, model, resp, options);
throw resp;
})
['catch'](function(resp) {
model.trigger('error', model, resp, options);
throw resp;
});
};

// Allow any `then`able (and `catch`able) to be considered a `Promise`
var isPromise = function(o) {
return o && (typeof o === 'object' || typeof o === 'function') && typeof o.then === 'function' && typeof o['catch'] === 'function';
}

return Backbone;

}));
Loading