Custom builds: use different ready code when excluding Deferred#2891
Custom builds: use different ready code when excluding Deferred#2891timmywil wants to merge 15 commits into
Conversation
|
By analyzing the blame information on this pull request, we identified @markelog, @mgol and @mikesherov to be potential reviewers |
There was a problem hiding this comment.
I'm pretty sure you can get rid of readyPromise and just use readyList directly.
- Keeps it Promise-compatible Fixes jquerygh-1778
| // Prevent errors from freezing future callback execution (gh-1823) | ||
| // Not backwards-compatible as this does not execute sync | ||
| window.setTimeout( function() { | ||
| fn.call( document, jQuery ); |
There was a problem hiding this comment.
Do we want to keep the document context? It's another backwards-compatible but non-Promise "feature". And if we do keep it, there should be verifying assertions in test/unit/ready.js.
There was a problem hiding this comment.
Perhaps we can leave that subject to change/undocumented. I don't want to verify it if we're going to change it in a future version.
There was a problem hiding this comment.
I'm suggesting that we change it now, in both places.
There was a problem hiding this comment.
Let's open another ticket then. This PR is already 2 tickets. I'd rather not hold it up. But I'm indifferent as to what context we use here. document still makes sense since it's the document ready handler.
There was a problem hiding this comment.
This PR is the introduction of jQuery.ready.then... why start off on the wrong foot when we don't have to? It doesn't even make sense to create a ticket referencing behavior that hasn't landed. But I guess I'll put some text here just in case:
Per Promises/A+ and ES2015,
thencallbacks should be called without athiscontext. And althoughjQuery.Deferred#*Withmethods provide context to callbacks, we are moving in the direction of increased standards compliance, and should stop providingdocumentcontext tojQuery.readycallbacks while we're already making backwards-incompatible changes tojQuery.ready.
There was a problem hiding this comment.
I'm fine with not passing document, we just need to document that and the fact that it's async now. I can do that in the release notes once this lands.
There was a problem hiding this comment.
@gibson042 The change wouldn't just affect jQuery.ready.then, and I'm familiar with the spec on Promises. Like you said, the document context is for back-compat, as we still have $(function(){}) and $(document).ready(), and while some may prefer Promise.resolve(jQuery.ready), I imagine that $(document).ready() and $(function() {}) will still be the most common, and a document context still makes sense to me on the latter 2. Like I said tho, I'm not opposed to changing that, but we don't have to change it now.
jQuery.ready.promisehas been removed andjQuery.readymade a thenable in core/ready..thenin the.readymethod.