CM_App.ajax: Return usable Promise

[vogdb]

Currently CM_App.ajax() returns the original jqXHR "Deferred" object.
It only rejects if the ajax request fails (e.g. network problem or non-200 response). But if we experience an expected application error (e.g. "auth required") it will resolve.

It would be great if ajax() would return a usable "Promise", that:

• Resolves when the request finished successfully (like success callback)
• Rejects when the request fails (like error callback)

We should also consider using ES6's Promise interface instead of jQuery's Promise: https://www.promisejs.org/#jquery

@vogdb @tomaszdurka thoughts?

[tomaszdurka]

I think it's great idea. We should definietly use es6-like solution.

• https://github.com/jakearchibald/es6-promise

[vogdb]

Should I branch here from master or conversation/chat-merge ?

[tomaszdurka]

Noes. I think it would be valuable to merge it into master asap.

[vogdb]

Current usages of cm.ajax do not assume that it returns anything like Promise. All the usages use callbacks to get the results. Should we replace callbacks with Promise interface ?
For example replace this:

    var xhr = cm.ajax('ajax', {viewInfoList: options.view.getViewInfoList(), method: functionName, params: params}, {
      success: function(response) {},
      error: function(msg, type, isPublic) {},
      complete: function() {}
    });

with that

    var xhr = cm.ajax('ajax', {viewInfoList: options.view.getViewInfoList(), method: functionName, params: params}).then(success, error);

Also you can see that there is no analog of complete callback in ES6's Promise.

[njam]

I would leave usages as they are, and migrate them step by step when we refactor code anyway.

Regarding complete - we could consider adding our own Promise.finally().
Did you already have a look at possible libraries/polyfills? Maybe some of them have it?

[tomaszdurka]

Depends how much work you want to do now.
I think it's also possible to use current signature and simply pass provided success/error callbacks to then/catch inside cm.ajax implementation.
Wdyt?

[vogdb]

There is no way to cancel es6-promise. The spec is not ready yet https://github.com/promises-aplus/cancellation-spec. We need cancellation for on('destruct', ...) cases. Also there is no cancellation for jQuery.Deferred. This https://github.com/petkaantonov/bluebird lib possibly fits all our needs.

[njam]

Hmm, this seems like a pretty specific use case. How about we handle it differently by allowing to pass in a closure argument to ajax(), à la:

promise = cm.ajax(..., {
 handleXhr: function(xhr) {
  this.on('destruct', function() {
   xhr.abort();
  });
 }
});

[vogdb]

[deleted because of the privacy]

[vogdb]

On the other hand bluebird library takes some space. 50kb in minified mode.

[vogdb]

Sorry that I multiply the comments here es6-promise takes 18kb.

[njam]

I don't necessary agree that adding ajax-specific features to Promise is elegant. One nice aspect of promises is that they can be combined: if a resolver returns another Promise it will be "chained".
If we start adding specific features to some promises, then they can't be mixed with other promises easily anymore.

I would therefore prefer to keep the Promise interface simple and standard, and expose ajax-specific features through other ways.

[vogdb]

cancel is not ajax-specific feature. The same goes for isPending. They are actually part of the drafts promises-aplus/cancellation-spec#1 and promises-aplus/synchronous-inspection-spec#3. They are not finished yet but their use cases are reasonable. Even from the point of view of Promise client it seems strange that I can't know its state to show on the progress bar or I can't cancel it.

[vogdb]

Actually I'm shocked by your position.

[vogdb]

There are 4 such places where we directly access underneath jqXHR. Do you plan to solve them by #1755? But how ? We still need to abort the current request.

[njam]

I agree, cancelation can be considered a generic Promise feature!

Unfortunately there's no consensus yet of how it should be implemented :/
Bluebird: A cancellable Promise (needs to be marked as such), can be cancelled by calling cancel(). It will reject the promise and throw Promise.CancellationError. Usage:

    return new Promise(function (resolve, reject) {
        xhr.send();
    }).cancellable().catch(Promise.CancellationError, function(e) {
        xhr.abort();
        throw e; //Don't swallow it
    });

There's also qajax, an ajax library on top of the Promise library Q.

[njam]

btw I just noticed CM_Form_Abstract.submit() returns a jQuery Promise, and uses ajax() under the hoods.

[vogdb]

Yes. Does that somehow affect your upper statements?

[njam]

Nope, jut a side note.

So should we try with Bluebird?

[vogdb]

I'm afraid of it cause it takes 50kb >_< but we don't have too much choice according to this research http://complexitymaze.com/2014/03/03/javascript-promises-a-comparison-of-libraries/ There are only when.js and qajax. when.js takes the same size as bluebird does. I want to try qajax. See how it behaves.

[njam]

Yup, and when.js has its "cancel" functionality marked deprecated as they wait for the official spec to be defined.

[tomaszdurka]

I think it's ok to use bluebird with its functionality for the moment.
On the other hand from time to time I think we might expect more than we should from Promise pattern and rather implement something on top of it.

[vogdb]

@tomaszdurka please check this TODO comment.

[vogdb]

For a formality. @tomaszdurka please review.

[tomaszdurka]

What is reason for adding this file? I understand these are instructions for build tool to include cancel.

I thought that our client vendor manager is very limited and that for now we only put end-files into the repo.

Wdyt?

[vogdb]

I think https://github.com/cargomedia/CM/blob/master/client-vendor/before-body/01-modernizr/config

[tomaszdurka]

I really like this distribution of the functionalities. Only thing we could change is naming of the methods.

showComponent sounds like it should take an existing component as argument. What about displayComponent or popOutComponent? Although I need to check behaviour of popOut.

[vogdb]

@tomaszdurka please review.

[tomaszdurka]

Looks good now.

I am still worried shouldn't we rather deal with cancelations in different way. E.g. with setting some flag on a Promise and then checking it's value within resolve.

[vogdb]

I'm afraid that the current mechanism does not allow this. Also if you get cancellation, you do not go to resolve clause => no point to wait in resolve cause resolve ain't going to be executed.

[tomaszdurka]

Oh I meant to not cancel/reject as we do via bluebird, but instead set this flag to let's say false and then resolve would not resolve :)

[vogdb]

But then you need to check for this flag in every resolve that is based on this promise?

[vogdb]

IMO they are equal

.catch(function(err){if(err instanceOf Cancellation){...}})

.then(function(result){if(result.flag){...}})

[tomaszdurka]

Looks good! Let's release this today.

One question, are we js-catching (non-promise) all the possible exceptions properly now? It would great to see (via js log) any kind of problems related with Promises once we release.
On the other hand ajax calls which are used for storing errors are now Promise-based.