Deferred: Backwards-compatible standards interoperability

[gibson042]

Ref #1821 (comment)

Maintained backwards compatibility (outside of specifications):

• Don't process resolution values: dfd.resolve( thenable ).done( expectsThenable )
• Multi-valued resolution: dfd.resolve( value1, value2, ... ).then( expectsMultipleArguments )
• Progress: dfd.notify( progress ).then( null, null, expectsInvocation )
• Contextual resolution: dfd.resolveWith( context, values ).then( expectsContext )

Necessarily-breaking changes

• Any return value (i.e., not thrown) from onRejected handlers can fulfill a .then Deferred: dfd.reject().then( null, jQuery.noop ).done( expectsInvocation )

Potentially-breaking behavior decisions (current state of this PR in bold):

• Thenables returned from .then onProgress handlers
  • progress: ignore vs. forward to outer onProgress vs. forward to outer onFulfilled vs. ???
  • rejection: throw (asynchronous) vs. ignore vs. forward to outer onRejected vs. ???
  • fulfillment: forward to outer onProgress vs. ignore vs. ???
• .pipe behavior: preserve as-is vs. support non-promiseable thenables
• .pipe support: deprecate vs. keep vs. drop
• Testing .pipe/.then incompatibilities: prefix tests with [DIVERGENT] vs. ???
• jQuery.when: privilege .promise (e.g., provide context) vs. privilege .then

To Do:

• update jQuery.when
• reduce size
• rebase (avoiding for now to keep a stable size impact reference)
• prototype unhandledRejections plugin

   raw     gz Sizes
252424  74450 dist/jquery.js
 85114  29662 dist/jquery.min.js

   raw     gz Compared to master @ 66e1b6b
 +5499  +1231 dist/jquery.js
  +921   +267 dist/jquery.min.js

[gibson042]

That piped deferreds inherit rejection is the reason why I couldn't justify dropping .pipe... the standard and our current behavior are just too divergent here.

[dmethvin]

Wow. I'll be reading through this today ... maybe tomorrow as well. :)

[dmethvin]

This is the part in the story where it swallows errors. Some of the libs do complicated "next-turn" checking but I wonder if we could just do a console.log if the error that was thrown is one of the developer-screwup errors like SyntaxError. Something as simple as this?

if ( /^(Eval|Internal|Range|Reference|Syntax|Type|URI)Error/.test(e) ) {
    console.log("[Caught exception] "+e);
    console.trace();
}

It could be in a public method that users could override perhaps?

Also note we only have one test case for thrown exceptions and none for browser-initiated exceptions.

[dmethvin]

This is really awesome. 👏 I want to understand it a little more, the spec references are helping.

[markelog]

👏 indeed

/cc @domenic

[dcherman]

It looks like the $.when implementation is still using .promise as a test to determine whether or not a given argument is a promise or not. Can that be changed for compatibility with other Promise libs, or is that out of scope for this PR?

[gibson042]

That's definitely in scope here. I'll update it.

[gibson042]

After some thought, I ended up nearly reversing the last block above... thenables returned from .then progress handlers are used only for their eventual fulfillment values (which translate back into progress values for the subordinate Deferred), with progress ignored and rejection generating (asynchronous) exceptions so their non-effect can be observed. It's possible to take a different direction, however, and I'd like other opinions—progress events, especially multiple independent chains of progress events originating from various points on a resolution chain, can generate the kinds of headaches usually reserved for time travel fiction.

Still to do:

• update jQuery.when
• reduce size
• rebase (avoiding for now to keep a stable size impact reference)
• prototype unhandledRejections plugin

[dmethvin]

I would be fine with corralling .then to only support standard Promise features, leaving back-compatible behavior to .pipe and the rest if that makes life easier.

[gibson042]

So that would involve .then resolved/rejected handlers always receiving exactly one argument and undefined context, and progress handlers never being called at all? We can do so, and it will simplify this effort, but I think it will mean committing to keep .pipe around for longer (since our internal uses will lean on it more heavily).

It's also a bigger backcompat break, but I don't know if that's a good thing or a bad thing here.

I'd like to further point out that these cases are just as potentially problematic with .pipe, it's just unlikely that anything ever comes close to exercising the full power of what we expose with progress—including our own unit tests. For example, I just found #2010.

[dcherman]

This may be more of a use case for jQuery.Migrate, but what about adding warnings in the unminified versions to warn the user if thenCallback.length or failCallback.length is > 1 when then is called since the standards compliant one will only accept 1 argument?

I'm just thinking about the nasty case of $.ajax where each callback accepts 3 arguments; the normal way to do that with Promises is to resolve it with a tuple and access the properties by index ( or with a spread helper that many libs provide ), however that's obviously a massive backcompat break.

[gibson042]

@jaubourg and @jquery/core:
I know we're all getting kind of antsy about Deferred/Promise interop, and having two competing PRs doesn't help. This one is basically ready to land now (with reduction deferred for later), but as I mentioned in the meeting, I'd like to focus more on the behavior changes and their reflection in unit tests than on the implementation. So please look over the description and test/unit/deferred.js and provide any comments you may have on the relevant points. Once we agree on them, the code can follow.

[benjamingr]

In order to deal with multiple arguments in then you can do what most promise libraries do and expose a .spread method in addition to .then that converts an array to variable arguments. Note that ES6 already ships with destructuring which accomplishes this.

[dcherman]

Just another point of concern with synchronous ajax calls:

I just took a look and unless I missed them, there are no unit tests that assert the ordering of callbacks. Since all handlers ( including the inline success, error, and complete options ) are attached using Deferreds, this change might have the effect of "breaking" synchronous ajax in that the callbacks will no longer be called synchronously.

[benjamingr]

@dcherman using deferreds with synchronous AJAX calls is not and has not been supported as of 1.8. Quoting the manual:

As of jQuery 1.8, the use of async: false with jqXHR ($.Deferred) is deprecated; you must use the success/error/complete callback options instead of the corresponding methods of the jqXHR object such as jqXHR.done() or the deprecated jqXHR.success().

[dcherman]

@benjamingr I did forget about that, but my point still stands actually unless I'm misunderstanding something:

https://github.com/jquery/jquery/blob/master/src/ajax.js#L490-L492
https://github.com/jquery/jquery/blob/master/src/ajax.js#L622-L624

The callback options just add the callbacks to the Deferred which is now resolved asynchronously, no?

[gibson042]

@dcherman: this change forces then callbacks to follow the on-object done/fail/always, by design. The actual sequence is that the latter fire first, synchronously in the order in which they were added, and then the former fire asynchronously in the order they were added. And the ajax module doesn't use then.

In short: only then is affected.

Do you foresee that causing problems? If so, I'm not sure anything can be done, since the asynchronicity is explicitly specified.

[dcherman]

@gibson042 Ah I see how it works now, I misunderstood how this change worked. Since as @benjamingr pointed it the Deferred/Promise methods have been deprecated for sync XHR since 1.8, it shouldn't officially cause a problem. I'm sure there are people out there that were depending on .then for sync, but there's not much that you can do about that and it's unsupported.

[timmywil]

@gibson042 How bout we land this?

[gibson042]

I didn't get a chance to merge this, but did clean up some text. At this point, I feel like it's good to go and the remaining work can be dealt with later.

[benjamingr]

Awesome! Great news

On Wed, Feb 25, 2015 at 7:25 PM, Richard Gibson notifications@github.com
wrote:

I didn't get a chance to merge this, but did clean up some text
gibson042@694374c.
At this point, I feel like it's good to go and the remaining work can be
dealt with later.

—
Reply to this email directly or view it on GitHub
#1996 (comment).

[timmywil]

Hey @gibson042, reminder to merge this when you get a chance. Thanks!