v2: Can of Promises #3729

au-phiware · 2015-07-24T12:29:12Z

I'm a big fan of Backbone and of Promises, this is my take on combining the two.

I understand that this may be a can of worms and I have changed many, many functions and it may not be your cup of tea. For this reason I have also made sure all the tests pass (with changes, of course) and I have tried to be backwards compatible (the Todos app still works). I haven't added any new tests yet but I would like to, as use cases arise and as I start to use this code in my own projects.

I have many use cases in mind and have already started with my own major changes: for instances Router#execute will catch errors and trigger an error event (on the router). My intention is that route actions should return the promise(s) from other methods (like render), this will mean that no error gets swallowed or forgotten about.

I'm making this PR, ignorant of the opinions of others (I've only just realised that there are already discussions about this) but keen to learn those opinions and to get some feedback.

I have made some assumptions:

Promise, as per ES6, is a global object. In my view it's very easy to include the standard polyfill.
jQuery.Deferred is not Promise and needs to be avoided.
Promises are useful outside of database interaction.

That last point is important to me. I am currently working on a big project that uses requirejs and pouchdb/couchdb. We use promises to orchestrate module dependencies as well as data from the server, whereby a Promise is created inside initialize and then is called inside render.

megawac · 2015-07-26T02:35:13Z

backbone.js

+      var success = options.success || _.identity;
+      var error = options.error;
+      delete options.success;
+      delete options.error;


Don't modify the options object

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

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

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

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.

megawac · 2015-07-26T02:44:25Z

I'm sympathetic to this changeset and think a movement like this may be a good way to move backbone forward (along with @jmeas' proposed router changes) in v2. So a hesitant 👍 after this has been carefully reviewed

@megawac

I know ECMA wouldn't break the internet, but one day `catch` might be a reserved word. On the suggestion of @megawac

I see no need to refer to `Backbone.Promise` everywhere since the reference from the arguments list isn't going to disappear.

@megawac

The Promise contract with Backbone is only two function, `then` and `catch`, that both return another `Promise`. On the suggestion of @megawac, see jashkenas#3729 (comment)

megawac · 2015-07-26T05:33:32Z

backbone.js

+    };
+    if (isPromise(defaults))
+      return defaults.then(consume);
+    return consume(defaults);


I'm not sure about returning a promise from initialize

I went there

👎 on this. Initialize is always synchronous, therefore this will always be resolved after you've created the instance -- in other words, exactly where you are with the current Backbone. Revert this please.

I have real world use cases where initialize is not synchronous. Of course, the constructor will not return a Promise if the user never returns a Promise... but I can tell that you feel strongly about this and I'm happy to agree to disagree.

Would love to hear those use cases if you feel like sharing.

On Mon, Jul 27, 2015, 9:06 AM Corin Lawson [email protected] wrote:

In backbone.js
#3729 (comment):

this.changed = {};

this.initialize.apply(this, arguments);

var model = this, args = slice.call(arguments);

var defaults = _.result(this, 'defaults');

var consume = function(defaults) {

attrs = _.defaults({}, attrs, defaults);

model.set(attrs, options);

model.changed = {};

var init = model.initialize.apply(model, args);

if (isPromise(init))

return init.then(_.constant(model));

return model;

};

if (isPromise(defaults))

return defaults.then(consume);

return consume(defaults);

I have real world use cases where initialize is not synchronous. Of
course, the constructor will not return a Promise if the user never
returns a Promise... but I can tell that you feel strongly about this and
I'm happy to agree to disagree.

—
Reply to this email directly or view it on GitHub
https://github.com/jashkenas/backbone/pull/3729/files#r35532739.

Adam K (mobile)

I think I'd rather let the user handle this themselves -- either with a custom wrapper or waiting for the promise to resolve

defaults.then(attrs => new Backbone.Model(attrs) .then(model => { });

I have reverted the commit that affects the constructors, and I hinted at a use case in the last sentence of my OP. I would like to create an example to illustrate why I started on this course in the first place, but that will take time to prune back some real world code into something bite-sited :)

See https://www.promisejs.org/

au-phiware · 2015-07-26T07:42:04Z

I get 7 failed tests when I use either this branch or the jashkenas/master branch.

jamesplease · 2015-07-26T13:45:23Z

backbone.js

  }

-}(function(root, Backbone, _, $) {
+}(function(root, Backbone, _, $, Promise) {


I'm not a fan of requiring a particular Promise implementation. Many people will end up requiring both the library you have chosen here and whatever Promise library they actually use.

In my libs I prefer doing:

if (typeof Promise !== 'undefined') { Backbone.Promise = Promise; }

That way, if someone's using native Promises, and they wish to continue using native promises, they can. Or if they'd rather use RSVP, they can do that, too. And so on.

An extra install instruction – telling people to configure Backbone.Promise – is worth the added benefit of flexibility, I think.

Yes, I agree... I felt dirty trying to pick an implementation from npm, bower, etc.

Agreed. Let's stick with the global Promise, or throw an error if one isn't defined (see @jridgewell's branch).

I removed the dependency on any one promise implementation but still attempt to load it via require (or fallback to global)... Do you see any problems with that?

Yes... if your project didn't include the promise library this bit of code would fail. It's much better to require the dev to provide a Promise polyfill or set one on Backbone.Promise directly instead.

but still attempt to load it via require (or fallback to global)... Do you see any problems with that?

Yeah, that is still going to be a problem for people. You should remove everything related to promises from both package.json, bower.json, and from the UMD wrapper.

Oh, I thought that the UMD wrapper would simply receive undefined... no probs

megawac · 2015-07-26T16:58:32Z

Also, there is significant overlap with #3651

@akre54

This reverts commit ecb606f at the suggestion of @akre54 see jashkenas#3729 (comment)

akre54 · 2015-07-27T15:04:03Z

I think this pull really showcases some of the downsides of Promises (and asynchronicity in your code in general) in that once you use them in a few places, you practically have to make your entire API asynchronous.

I see very little reason for Model#defaults or Router#execute, to give two examples, to be asynchronous. That's not the right place for network or user-facing code to exist. You're contorting Backbone's API — and by extension your code — to fit a small assortment of problems that are better handled elsewhere.

I'm fully in support of wrapping asynchronous calls (sync is the biggest one) in Promises, however.

au-phiware · 2015-07-28T01:47:01Z

Ha! Perhaps I should have added: "WARNING: May cause Scoliosis"

But on a serious note, I don't like the isPromise checks but I think there's power in giving the option to have a value, a function or a Promise at those points. IMO, writing with Promises demands a different way of thinking. Remember that a Promise does not represent an async call, instead it is simply a dependency on an async call, which could have been fulfilled at any poiny in the past. So it is not correct to assume that whereever there is a Promise there will be a network call or something else that hasn't happen.

So I agree that defaults is not the right place to be accessing the network, etc. But I can't dictate that it can't be time dependent on something else.

au-phiware · 2015-07-28T01:52:47Z

Sorry, that close was a slip of the thumb...

akre54 · 2015-07-28T02:22:26Z

Scoliosis

Ha. @paulmillr made a fork of Backbone a while back called Scoliosis (since renamed Exoskeleton). Maybe we could implement it there first.

Remember that a Promise does not represent an async call, instead it is simply a dependency on an async call, which could have been fulfilled at any poiny in the past.

Right of course. But it starts to get ugly fast when you mix sync and async code using promises. In my experience they tend to take over your entire application when really you should've been writing it reactively in the first place.

I don't like the isPromise checks but I think there's power in giving the option to have a value, a function or a Promise at those points.

There's power but there's also a great deal of sloppiness. You know what Ben Parker (and Teddy Roosevelt) said about power and responsibility...

So I agree that defaults is not the right place to be accessing the network, etc. But I can't dictate that it can't be time dependent on something else.

Can you give an example?

godlark · 2015-08-28T14:49:05Z

I think if we want to return a promise it would be good to pass parsed response to then's callback. Read more at #3779

prantlf · 2018-01-07T23:27:34Z

The sync method in Backbone 1.x returns promise using the interface of jQuery.Deferred. Be careful with switching to Promise/A interface in the result of this method. It would be a breaking change. You should schedule it for Backbone 2.x.

au-phiware added 7 commits July 21, 2015 22:35

sync returns a Promise

bfa268c

Play nice with callbacks.

e9f019e

Any constructor or may provide a Promise.

ecb606f

Merge branch 'master' of https://github.com/jashkenas/backbone

91227ee

Router#execute may provide a Promise or trigger an error.

9adf677

Permit routers to store metadata in History's handlers.

5b39cf9

Merge branch 'master' of https://github.com/jashkenas/backbone

bdaa11b

megawac reviewed Jul 26, 2015
View reviewed changes

au-phiware added 5 commits July 26, 2015 13:23

Avoid conflict with potential reserved word.

ca27d88

I know ECMA wouldn't break the internet, but one day `catch` might be a reserved word. On the suggestion of @megawac

Don't assume Promise is a global.

53e9555

I see no need to refer to `Backbone.Promise` everywhere since the reference from the arguments list isn't going to disappear.

Relax the definition of a Promise.

0d4fe5b

The Promise contract with Backbone is only two function, `then` and `catch`, that both return another `Promise`. On the suggestion of @megawac, see jashkenas#3729 (comment)

Reject with an error when the error is known.

0bee4c0

Event listener errors are handled by the caller of save, fetch, destroy.

403bc44

megawac reviewed Jul 26, 2015
View reviewed changes

au-phiware added 2 commits July 26, 2015 16:35

Fix typo

fb45026

Depend on promise.js.

820dd25

See https://www.promisejs.org/

jamesplease reviewed Jul 26, 2015
View reviewed changes

au-phiware added 3 commits July 27, 2015 22:55

Don't prescribe Promise implementation and fallback to global.

509adab

Clarify the extent of the use of Promises.

747ffaa

Revert "Any constructor or may provide a Promise."

7eddccd

This reverts commit ecb606f at the suggestion of @akre54 see jashkenas#3729 (comment)

au-phiware added 2 commits July 27, 2015 23:39

Remove my 'mission statement'.

2e8c79c

Model#save rejects with validationError that is created by Model#set.

0833ee7

au-phiware added 2 commits July 28, 2015 01:22

Omit Promise from UMD wrapper.

44bec7b

Return callback result for correct event propagation.

667f81d

au-phiware closed this Jul 28, 2015

au-phiware reopened this Jul 28, 2015

Merge branch 'master' of https://github.com/jashkenas/backbone

21ab2ee

akre54 mentioned this pull request Aug 27, 2015

model.fetch.then #3779

Closed

akre54 mentioned this pull request Nov 14, 2016

Promise as argument of Collection add, remove, reset, set methods #4096

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2: Can of Promises #3729

v2: Can of Promises #3729

au-phiware commented Jul 24, 2015

megawac Jul 26, 2015

au-phiware Jul 26, 2015

au-phiware Jul 26, 2015

akre54 Jul 27, 2015

au-phiware Jul 27, 2015

megawac commented Jul 26, 2015

megawac Jul 26, 2015

au-phiware Jul 26, 2015

akre54 Jul 27, 2015

au-phiware Jul 27, 2015

akre54 Jul 27, 2015

megawac Jul 27, 2015

au-phiware Jul 27, 2015

au-phiware commented Jul 26, 2015

jamesplease Jul 26, 2015

au-phiware Jul 26, 2015

akre54 Jul 27, 2015

au-phiware Jul 27, 2015

akre54 Jul 27, 2015

jamesplease Jul 27, 2015

au-phiware Jul 27, 2015

megawac commented Jul 26, 2015

akre54 commented Jul 27, 2015

au-phiware commented Jul 28, 2015

au-phiware commented Jul 28, 2015

akre54 commented Jul 28, 2015

godlark commented Aug 28, 2015

prantlf commented Jan 7, 2018

v2: Can of Promises #3729

Are you sure you want to change the base?

v2: Can of Promises #3729

Conversation

au-phiware commented Jul 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megawac commented Jul 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

au-phiware commented Jul 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megawac commented Jul 26, 2015

akre54 commented Jul 27, 2015

au-phiware commented Jul 28, 2015

au-phiware commented Jul 28, 2015

akre54 commented Jul 28, 2015

godlark commented Aug 28, 2015

prantlf commented Jan 7, 2018