Does not work with jQuery Promises #1

Closed
nonplus opened this Issue Apr 12, 2012 · 16 comments

Comments

Projects
None yet
3 participants

nonplus commented Apr 12, 2012

You currently can't use chai-as-promised with jQuery's Deferred Promises. Specifically, the rejected assertions don't returned a fulfilled promise and the following will fail on the second assert:

var assertPromise = assert.isRejected(jQuery.Deferred().reject(new Error("fail!")));
assert.isFulfilled(assertPromise, "isRejected returns fulfilled promise");

The reason for this is that unlike the CommonJS Promises/A specification, jQuery's then() method returns the original promise rather than a new promise.

jQuery has a pipe() method which is similar to the Promises/A's then(). Changing the rejectedAsserter to use the following makes it work correctly for jQuery.

var transformedPromise = Object.create(this.obj.pipe(onOriginalFulfilled, onOriginalRejected));

It would be great if chai-as-promised were aware of jQuery's behavior or if the creation of the transformedPromise was pluggable s.t. jQuery tests could implement it using pipe().

Owner

domenic commented Apr 12, 2012

I knew this day would come -_-. My gut instinct is to say "I only support CommonJS Promises/A, sorry." But just like I should be nice to retarded children, I probably should also welcome jQuery promises. (In my own code, I immediately wrap them with Q.when, kind of like how you use those little bags when your otherwise-cute dog shits in the park.)

It seems like the best approach would be to detect the presence of a pipe method, and if present, use it in preference to then. Such an unfortunate hack. It doesn't just affect the one line you found; it looks like there's seven places I use then assuming it will actually work.

Regardless, if I want to make this work, I'll have to make it work right. That means creating a version of the test suite that uses jQuery promises instead of Q ones, and making sure it all passes. Sounds like at least a weekend project.

Thanks for bringing it up. As much as it makes me sad, a lot of jQuery promises are floating around out there, and I can't assume everyone will be using protection.

nonplus commented Apr 12, 2012

Heh, I know exactly how you feel. :-)

FWIW, I don't really expect jQuery deferred support to be baked into chai-as-promised, but it would be nice if you could expose hooks s.t. someone could inject their own implementation. The default implementation should work for CommonJS Promise/A promises.

For example, you could add an invokeThen(promise, ok, err, notify) property to chaiAsPromised and inside your code replace calls to promise.then(...) with invokeThen(promise, ...).

The default (Promises/A) implementation would be something like this:

function(promise, ok, err, notify) {
  return promise.then(ok, err, notify);
}

People that use jQuery could then override it like so:

chaiAsPromised.invokeThen = function(promise, ok, err, notify) {
    return promise.pipe(ok, err, notify);
}

Also, you mention using Q for promises. Any reason why use Q rather than when? Seeing as when implements Promises/A whereas Q implements Promises/B (not that it matters for chai-as-promised).

Owner

domenic commented Apr 12, 2012

Yeah, I was thinking of the hook approach, but in the end, it's such a small hook I might as well include it. I think the hooks would be more useful for a situation where it has to be either-or, i.e. if supporting jQuery promises precluded supporting CommonJS ones. For this situation, I can include both with no sacrifice.

Q actually implements multiple promise specs, I believe including both A and B. There used to be a list in the docs; @kriskowal, care to chime in?

So my choice of Q comes down mainly to preference for the little things. E.g. the cleaner separation of deferreds and promises, and the much stronger set of promise methods (see "Over the Wire" in the Q readme). Generally there's more sugar (spread, fin, fail, all the Node helpers, plus the upcoming try/catch/finally substitutes for call/fail/fin).

invokeThen is equivalent to Q.when, and Q.when works on jQuery promises since it assimilates any thenable promise. If you copy Q.when to your interface, it could be replaced with another, but I don’t think that would even be necessary. Just assimilate!

Q implements CommonJS/Promises/{A,B,D}.

Owner

domenic commented Apr 12, 2012

Yeah, one route I was thinking was to take on an (optional?) Q dependency and just assimilate the promise being asserted about ASAP. Hmm.

nonplus commented Apr 12, 2012

I think it's a bad idea for chai-as-promised to always assimilate the passed in promise. If I'm a jQuery deferred user, I might expect (or rely on) the promise returned by your asserts to be a jQuery promise s.t. I can use the jQuery specific methods (like fail(), done() and always()). If I wanted an assimilated promise, I would do the assimilation explicitly in my test, e.g. assert.isRejected(Q.when(jQueryPromise))...

Exposing this as a hook allows people to do what they want to do, rather than what you think they want to (or ought to) do. I think it's OK to expect people that use non-standard promises to jump through some hoops.

Owner

domenic commented Apr 21, 2012

The good news is, I have a pretty sweet automated browser-testing script written, with the ability to switch between arbitrary promise implementations.

The bad news is, it appears when.js also has the jQuery-style deficiency? And they don't even have a pipe method to make up for it? That makes assimilation seem more attractive. I was looking at an outdated fork; when.js passes with flying colors.

domenic added a commit that referenced this issue Apr 21, 2012

New semi-automated browser tests for Q, when.js, and jQuery.
Run them with `npm run-script test-browser-q`, `npm-run-script test-browser-when`, and `npm-run-script test-browser-jquery`.

Q and when.js pass with flying colors. jQuery fails many, many tests (tracking as GH-1).
Owner

domenic commented Apr 21, 2012

Even if you replace then with pipe, the new test suite reveals a host of problems that I'm not sure how to solve. For one, you cannot recover from errors in jQuery: they immediately terminate the promise chain, even when using pipe.

jQuery.when(42).pipe(function () { throw Error(); })
               .pipe(function () { console.log("ok"); });
// => uncaught Error, not "ok".

If anyone wants to help debug this, I've put the test infrastructure in place to make your job a decent bit easier. But I'm probably not going to spend any more time on it, since it appears to not be a very easy fix.

I suppose I could detect the presence of jQuery promises in particular and then assimilate them using Q, throwing an error if Q isn't included on the page.

nonplus commented Apr 23, 2012

FYI, people using jQuery's deferred are presumably aware of how it (doesn't) address thrown exceptions and their code accounts for this (FWIW, we use a helper function that catches exceptions and converts them to rejected promises).

If you wanted to, chai-as-promised could wrap the success/error callbacks into a try-catch and if the callback throws an exception assert failure. However, it still won't solve the problem with trying to have a generic unit test against both CommonJS and jQuery promises, but they inherently behave differently.

To clarify, when I opened this case, I was primarily interested in Chai returning pipeable promises. For my needs this involved a one-line change to chai - I've been happily using a hacked version in my websql.js project which also happens to make the promise provider pluggable.

Owner

domenic commented Apr 25, 2012

The problem is, the ability to trap thrown exceptions is necessary for some very basic Chai as Promises stuff, as currently implemented. If you make the one-line change and run the unit tests against jQuery (npm run-script test-browser-jquery), you'll see that the vast majority of Chai as Promised is broken. I'm curious what kind of tests you are running wherein the one-line change version is acceptable.

nonplus commented Apr 25, 2012

Yes, I'd expect your unit tests to fail. Like I said in my previous comment, the exception behavior in jQuery and CommonJS is inherently different and you can't expect the exact same unit test to work for both.

Since our code has to deal with jQuery's crappy handling of exceptions, we always make sure that our functions that return a promise catch any exceptions and convert them into a rejection - basically we have to do what CommonJS promises automatically do. So in our unit tests, the promise assert callbacks never throw an exception and always return a proper valueOrPromise.

So when running unit tests for my jQuery promise code, exceptions thrown from callbacks never come into play.

FYI, we use the following to wrap code that might be throwing an exception:

function deferredTryCatch(deferredCallback) {
    try {
        return deferredCallback();
    } catch(err) {
        return $.Deferred().reject(err);
    }
}

function asyncFunction(...) {
    return deferredTryCatch(function() {
        // Code in here can return a value, promise or safely throw an exception
        ...
    });
}
Owner

domenic commented Apr 25, 2012

Right, but my unit tests don't depend on this behavior---Chai as Promised itself does, e.g. in implementing .rejected.with or .eventually.. My unit tests indicate that the majority of Chai as Promised cannot be implemented in a promise system that does not trap thrown exceptions (namely AssertionErrors).

Owner

domenic commented Apr 25, 2012

Remember, these are the unit tests for Chai as Promised itself---if they fail in a given environment, that means Chai as Promised is broken in that environment.

Perhaps you can provide two hooks. One for importing promises, one for exporting.

Owner

domenic commented Apr 26, 2012

@kriskowal I would also need to wrap all instances of assert in some kind of try { } catch (e) { return exportRejectedPromise(e); }. This would be difficult to factor out into a wrapper since I need to exit the calling scope iff assert fails. (A use case for TCP, heh.)

I tried to see if the jQuery guys were open to fixing their implementation, but it looks like while jQuery 1.8 will have a then method that chains, it will not respect Promises/A :(. http://bugs.jquery.com/ticket/11010

Owner

domenic commented May 22, 2012

jQuery promises explicitly rejected as of c158153 (3.1.0).

@domenic domenic closed this May 22, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment