-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Custom builds: use different ready code when excluding Deferred #2891
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
Conversation
|
By analyzing the blame information on this pull request, we identified @markelog, @mgol and @mikesherov to be potential reviewers |
src/core/ready.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting that we change it now, in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can accept that: gh-3026.
jQuery.ready.promisehas been removed andjQuery.readymade a thenable in core/ready..thenin the.readymethod.