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.

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

Improve API interoperability with standard Promise #1722

Closed
mgol opened this issue Oct 20, 2014 · 38 comments
Closed

Improve API interoperability with standard Promise #1722

mgol opened this issue Oct 20, 2014 · 38 comments

Comments

@mgol
Copy link
Member

mgol commented Oct 20, 2014

Originally reported by @jzaefferer at: http://bugs.jquery.com/ticket/14510

Discussed this at the jQuery team meeting in Amsterdam: The spec is still changing a lot (within whatwg and draft pages on github), so we'll wait for it to ship, unprefixed, not behind a flag, in stable browsers first.

Once that happened, we should change/fix our implementation to match the spec (and shipped implementation).

Can use .pipe() to continue using any jQuery-specific functionality.

@mgol mgol added this to the 3.0.0 milestone Oct 20, 2014
@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dj.gilcrease@…

Make jQuery.Defered / promise closer to conforming to the specification around exceptions. I have not checked the full compliance tests from  https://github.com/promises-aplus/promises-tests but I have setup a test that shows the exception handling working as expected  http://jsfiddle.net/yEXL4/1/

Pull Request @  https://github.com/jquery/jquery/pull/1462

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

I don't think this is the right way to go. You're swallowing an exception silently and it will only be reported (poorly) if the user has the foresight to attach a fail handler. See #11193, #11292.

Even in the current proposed spec discussions there's some handwaving about needing browser support in dev tools to be able to debug properly.

What I'd prefer to do is have it so that users can opt into standard ES6 Promises or a compliant polyfill. Retroactively changing the semantics of Deferred in a significant way seems like a bad idea, especially with no built-in support to notify about unhandled promise rejections.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: markelog

This ticket requires clarification.

Specification that mentioned in ticket description is  this one which refers to Domenic  repository.

There is also  promises/A and  promises/A+ "proposals". Which are not much of a relevance right now. Plus upcoming draft of ES6 which should also have Promise clause.

DOM specification is in the draft stage, but it's already implemented in two browsers – Firefox (Nightly) and Chrome (Canary) without any prefixes, both browsers however has some implementation mistakes.

New specification defines new API which significantly differs from the jQuery Deferred, aside from methods like "cast", "catch", etc; it also has these behaivor differences:

  1. Guarantee asynchronous execution
  2. Swallows errors then passes and propagate them to the stack of rejection handlers
  3. Does not have progress handler in "then" signature
  4. Always passes only one argument to successful/failed callbacks

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

It's definitely good to emphasize that Promises/A+ is just a proposal and not a standard to comply with. jQuery has had Deferred for several years now and I would guess it has the most number of users of any Promise/Deferred implementation that exists today. Let's not break that code, keep Deferred the way it is, and look for ways to let developers use the emerging standard.

There has been some  lively discussion around the addition of a .done() method to ES6 Promises that would not swallow exceptions, but it doesn't seem to be slated for the first iteration of the standard. I think that's a shame, because .then() can be built from .done() but not the other way around. Without the proposed browser support for exposing unhandled exceptions, Promises are going to be very difficult to debug.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: rwaldron

Replying to markelog:

This ticket requires clarification.

Specification that mentioned in ticket description is  this one which refers to Domenic  repository.

This is the Promises specification that will be in ES6

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: domenic@…

Promises/A+ is a standard and is the basis for the ES6 promises specification. All promises that follow the ES6 promise specification will pass all Promises/A+ tests. The ES6 promises spec is in effect a superset of the Promises/A+ spec.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: anonymous

.then() can be built from .done() but not the other way around.

This is false.

Promise.prototype.done = (onFulfilled, onRejected) {
  this.then(onFulfilled, onRejected).catch(e => setTimeout(() => throw e, 0));
};

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

Re-throwing an error object that has propagated to you isn't the same as letting the browser handle the error at the point where it occurred. A lot of important error context is lost, especially on older browsers (which 1.x still supports).

However, that particular pull request listed above is about trying to turn Deferred into Promises/A+. We can't do that, it would break a few years worth of existing code that is already using Deferred. Nobody wants to break the web. Instead we expect to support ES6 Promises like I mentioned above. That seems like a good approach?

EDIT: Clarified the subject; the reason this ticket isn't closed is because we expect to support ES6 Promises but are not changing Deferred.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: domenic@…

I guess it's unclear what the OP meant by

Make promises spec-compliant

... we should change/fix our implementation to match the spec (and shipped implementation).

Can use .pipe() to continue using any jQuery-specific functionality.

It sounds like you are saying something different from the OP, wherein you do not change/fix your implementation to match the spec, but instead somehow support real promises (unclear how).

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

Reading through this, I agree it's unclear. The pull request referencing this issue was misplaced, since the OP doesn't mention changing our Deferred at all. If it becomes too confusing we can close this and open a new ticket with a more specific title.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: m_gol

I was at the Amsterdam meeting where it was discussed. OP says about compliance with ES6 spec. If wording is not clear, we can modify title & text of the issue but I'd prefer not to re-create one since discussion already started here.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: m_gol

We can discuss it in San Diego but, surprisingly, jaubourg seemed ok to switch once at least 2 stable browsers implement the new spec while keeping pipe with current semantics.

I'd like to see how much code would be affected, I don't think many people relied (in the production code) on the promise throwing errors as a way of flow control. OTH, for jQuery even a small amount can be a lot.

There's a larger question of how we deprecate old APIs, e.g. in case of $.ajax and the planned $.xhr. Will we ever be able to actually remove the former? The question here is of similar concern.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: calvin.metcalf@…

Chrome beta implements them and Firefox beta has them out from under a flag, so 2 stable browsers will be soon.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: markelog

Replying to calvin.metcalf@…:

Chrome beta implements them and Firefox beta has them out from under a flag, so 2 stable browsers will be soon.

See ticket description – "so we'll wait for it to ship, unprefixed, not behind a flag, in stable browsers first"

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: anonymous

Unwrapping thenables is also a change you should consider doing. It's in the spec and it's useful.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

I've edited the title to be more in line with what we actually plan to do for 1.12/2.2. We are not converting our internal uses of $.Deferred to the emerging Promise implementations, the semantics of the two differ too much and Promise lacks several features that $.Deferred uses, such as progress callbacks and multiple arguments.

However, we do plan on accepting Promise as an input where appropriate (e.g., $.when). and ensuring that $.Deferred works as a thenable.

@benjamingr
Copy link

Please consider when you're executing a handler to deferred. One of the thing every A+ promise does is always defer execution of handlers so:

myPromise.then(function(){
    log("World");
});
log("Hello ");

Will always log "Hello World" on an A+ promise implementation. jQuery on the other hand will log either "Hello World" or "World Hello" sometimes based on whether or not the promise resolved before or after the then was called.

This has been a steady source of bugs where developers rely on the order and are exposed to race conditions. I think fixing this mistake in the next major revision of jQuery could really help a lot of developers and help prevent race condition related bugs.

@dmethvin
Copy link
Member

dmethvin commented Jan 7, 2015

@benjamingr The changes in #1996 bring .then up to Promise/A+, we run the test suite to be sure. We should be landing that soon.

@benjamingr
Copy link

@dmethvin that landing in jQuery would be awesome - does this means you'll also address catch correctly? This is huge - I answer promise questions on StackOverflow pretty often and bringing then up to A+ standard would help a ton of people.

I think I got confused by the issue date because it was migrated.

@dmethvin
Copy link
Member

dmethvin commented Jan 7, 2015

does this means you'll also address catch correctly?

You mean do we turn exceptions into rejections rather than allowing them to be uncaught? Yes, although we're working on how to report those in some reasonably visible way. It's related to the issue you opened in petkaantonov/bluebird#428, it's just too easy for programming errors to be lost and you see this problem over and over when people start working with Promises. Now imagine that someone drops a Promise-heavy piece of code in their web page that was written by a third party and has some problem. They won't know why it's just not working but has no visible errors.

@benjamingr
Copy link

Bluebird does a pretty good job of detecting unhandled rejections (a simple heuristic is that those are promises you don't add done or a catch handler to in this or the next turn).

I've recently made a survey on how people use Bluebird's onPossiblyUnhandledRejection hook (which lets you determine what to do when the library thinks there's a rejection) - the vast majority of people stick to the default behavior which is to log the error the console when the developer can see it. Here's an example.

Note that bluebird does more than just track the rejections (it stitches the stack traces, wraps primitives implicitly for logging and has a much bigger api etc) and you have different important considerations in jQuery (such as file size). So you might not want to implement all the debugging stuff that's in bluebird. Honestly just doing A+ compliance itself is huge and doing A+ compliance and unhandled rejection handling is crossing the line between good news for interoperability and being able to use jQuery as the main promise implementation in a frontend project.

@paulirish
Copy link
Member

Just FYI, There's a good amount of tooling for native promises.

asynchronous callstacks (already available)

dedicated promises panel (coming soon)

image

I think it's worth considering using the native Promise impl and falling back to the Deferred code for older browsers. Developers would end up with a more enjoyable debugging experience.

@dmethvin
Copy link
Member

We're not removing any jQuery.Deferred functionality, which we use internally, and the native Promise doesn't have those extensions. Our goal here was to provide standard Promise/A+ behavior for .then() and not to implement a native Promise shim. If someone wants native Promise and decent debugging for it is supported on all the platforms they want to use, they should definitely do that instead. It will be a while before platforms like IE8-11 and Android 2.x die, so if people want cross-browser behavior they can use jQuery.Deferred.

If someone has a better plan for how to do this, please let us know.

@benjamingr
Copy link

@paulirish how can a non-native promise hook into that tooling?

@markelog
Copy link
Member

@dmethvin i guess @paulirish meant that we can use native Promises if they available, then enhance/extend them with our methods.

If i interpreted that right, this would create two code-paths which would increase the size and performance might suffer too (judging by https://github.com/petkaantonov/bluebird/tree/master/benchmark), although i'm not sure if that matters in browser envs or that we can preserve consistency between those routes.

And, as i understand, the only reason for us to consider it is to improve debug process?

@benjamingr
Copy link

@markelog native promises are really slow but they're still likely much much faster than jQuery promises - at least today.

@markelog
Copy link
Member

@markelog native promises are really slow but they're still likely much much faster than jQuery promises

Pretty sure this is not accurate, jQuery.Deferred, currently, should be fastest promises out there, since they don't guarantee async execution and therefore don't wait for next event loop iteration.

@benjamingr
Copy link

@markelog I'm think you might want to run the benchmarks on that. For fun and glory you can also run Bluebird against itself with the sync (zalgo) build (that does not guarantee async operation).

Bluebird's petkaantonov actually wrote the sync build just to prove that the performance benefits of not guaranteeing async execution are marginal at best. The trick is that instead of doing something native like using a setTimeout every time you want to create async behavior you queue the pending async executions together and batch them in the next turn.

So gnerally I'm pretty sure that Bluebird is at least two orders of magnitude faster than jQuery deferreds at the moment. Native promises using the microtask queue are probably at least one order of magnitude faster than jQuery - feel free to benchmark those.

@markelog
Copy link
Member

I'm think you might want to run the benchmarks on that.

Since imp of jQuery.Deferred will be compatable with Promise/A+ there is no need for that.

The trick is that instead of doing something native like using a setTimeout every time you want to create async behavior you queue the pending async executions together and batch them in the next turn.

This is popular trick which used in many Promises libs, but you still need to wait for next iteration.

So gnerally I'm pretty sure that Bluebird is at least two orders of magnitude faster than jQuery deferreds at the moment.

I see the conclusion not the argument which should lead to it.

Native promises using the microtask queue are probably at least one order of magnitude faster than jQuery

If we would talk about microtasks vs macrotasks that would be relevant, but jQuery.Deferred doens't require non of those, code that doesn't require async execution is always faster then alternative.

feel free to benchmark those.

Besides the points i made above, it doesn't really fair to compare async vs sync execution, since one should be inherently faster then the other.

@benjamingr
Copy link

@markelog I'll try to be clearer and to the point: Bluebird promises will run circles around jQuery defererds any day even while jQuery uses a sync ("zalgo") execution model (which is thankfully being fixed). This is because bluebird promises consume less memory than jQuery deferred by using flags, less state variables and no closures.

Here is a trivial microbenchmark in which no async execution is involved at all (so bluebird defers here and jQuery performs nothing asynhronous). This is not a typical load - it's a completely synchronous code: http://jsperf.com/bluebird-vs-jquery-promises

Bluebird performs 10 times faster than jQuery here, and this is the kind of benchmark sync execution is supposed to shine - again, nothing is actually asynchronous here.

For what it's worth even when you disable the async A+ deferral in Bluebird it is only marginally faster.

@dmethvin
Copy link
Member

Our main focus here is interop per the topic. If someone has perf issues and wants to use Bluebird or native Promise they can do so. I really do like native Promise but don't think the 90% of people who use the monolithic jQuery file would appreciate having the weight of two implementations just in order to use them. We have to solve the problem decently for all the platforms we support, all the way back to IE8 and Android 2.3.

@markelog
Copy link
Member

nothing is actually asynchronous here.

Tests are, you force benchmark.js execute your tests in async manner i.e. putting them in setTimeout call and therefore make jQuery test report later.

I really don't want to argue about this, implementation of jQuery.Deferred will change, bluebird has a reputation of crazy optimizations and considered one of the most fastest Promise/A+ impl, so i don't understand what you trying to prove here.

@markelog
Copy link
Member

and btw

For what it's worth even when you disable the async A+ deferral in Bluebird it is only marginally faster.

It's not, or shouldn't be, benchmark.js couldn't give exact ops number for async tests, so this diff should fall on margin of error type of thing

@paulirish
Copy link
Member

@dmethvin sorry for helping derail the conversation. :)

Oleg understood where I was going with this and.. yes it would require a feature detect and forked codepath (yay, how jQuery!) but would likely end up adding more code complexity (boo!). I think the jQuery position on this kind of thing has been mixed in the past. Users will miss out on the debugging help from tools, but I understand it's not trivial for you to handle this otherwise.

@markelog @benjamingr I carried this conversation about promises performance over here: https://plus.google.com/+PaulIrish/posts/ZBP64F2GEY4 Would love both your feedback.

gibson042 added a commit that referenced this issue Mar 20, 2015
@benjamingr
Copy link

Woohoo!! When can we expect this to be in the release?

@gibson042
Copy link
Member

It's already in git builds of compat (formerly 1.x) and (formerly 2.x), and will be in both 3.0 betas following the resolution of our blockers, which will be soon since this was the biggest of them.

@benjamingr
Copy link

Awesome, thanks a ton :)

@mgol
Copy link
Member Author

mgol commented Nov 3, 2015

The "Behavior Change" label was missing from this ticket - do we have more of those? We should pay attention so that all such tickets have this label applied.

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

No branches or pull requests

7 participants