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

Custom builds: use different ready code when excluding Deferred #2891

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@timmywil
Member

timmywil commented Feb 1, 2016

  • Modules depending on deferred are excluded when deferred is excluded (ajax, effects, queue, and core/ready). Whenever deferred is excluded, core/ready is replaced by core/ready-no-deferred.
  • jQuery.ready.promise has been removed and jQuery.ready made a thenable in core/ready.
  • #1823 (error resiliency) has been addressed by giving up our synchronicity guarantee in ready handler execution after the ready event has been fired. This has multiple advantages. Besides error resiliency, it makes use of our standards-compatible .then in the .ready method.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 1, 2016

By analyzing the blame information on this pull request, we identified @markelog, @mgol and @mikesherov to be potential reviewers

mention-bot commented Feb 1, 2016

By analyzing the blame information on this pull request, we identified @markelog, @mgol and @mikesherov to be potential reviewers

Show outdated Hide outdated src/core/ready.js
@@ -5,12 +5,13 @@ define( [
], function( jQuery, document ) {
// The deferred used on DOM ready
var readyList;
var readyList = jQuery.Deferred(),
readyPromise = readyList.promise();

This comment has been minimized.

@gibson042

gibson042 Feb 3, 2016

Member

I'm pretty sure you can get rid of readyPromise and just use readyList directly.

@gibson042

gibson042 Feb 3, 2016

Member

I'm pretty sure you can get rid of readyPromise and just use readyList directly.

Show outdated Hide outdated src/core/ready.js
jQuery.fn.ready = function( fn ) {
// Add the callback
jQuery.ready.promise().done( fn );
readyPromise.done( fn );

This comment has been minimized.

@gibson042

gibson042 Feb 3, 2016

Member

This .done is the source of #1823... if we're willing to give up a synchronicity guarantee, then it could become .then, the synchronous invocations could get wrapped in setTimeout (removing try...finally in the process), and those error resiliency tests could be restored.

What do you think? Is anyone counting on jQuery( invokedFirst ); invokedSecond()?

@gibson042

gibson042 Feb 3, 2016

Member

This .done is the source of #1823... if we're willing to give up a synchronicity guarantee, then it could become .then, the synchronous invocations could get wrapped in setTimeout (removing try...finally in the process), and those error resiliency tests could be restored.

What do you think? Is anyone counting on jQuery( invokedFirst ); invokedSecond()?

This comment has been minimized.

@mgol

mgol Feb 3, 2016

Member

I think it's much less likely to depend on that than to depend on synchronous then on a resolved promise and yet we made that change. Aligning those behaviors only seems good to me.

@mgol

mgol Feb 3, 2016

Member

I think it's much less likely to depend on that than to depend on synchronous then on a resolved promise and yet we made that change. Aligning those behaviors only seems good to me.

This comment has been minimized.

@timmywil

timmywil Feb 4, 2016

Member

I actually tried switching to then, but didn't get the behavior I expected. I'll keep digging.

@timmywil

timmywil Feb 4, 2016

Member

I actually tried switching to then, but didn't get the behavior I expected. I'll keep digging.

This comment has been minimized.

@mgol

mgol Mar 21, 2016

Member

@timmywil Did you find sth?

@mgol

mgol Mar 21, 2016

Member

@timmywil Did you find sth?

This comment has been minimized.

@timmywil

timmywil Mar 21, 2016

Member

@mgol I'll work on it this week.

@timmywil

timmywil Mar 21, 2016

Member

@mgol I'll work on it this week.

// 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 );

This comment has been minimized.

@gibson042

gibson042 Mar 28, 2016

Member

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.

@gibson042

gibson042 Mar 28, 2016

Member

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.

This comment has been minimized.

@timmywil

timmywil Mar 28, 2016

Member

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.

@timmywil

timmywil Mar 28, 2016

Member

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.

This comment has been minimized.

@gibson042

gibson042 Mar 28, 2016

Member

I'm suggesting that we change it now, in both places.

@gibson042

gibson042 Mar 28, 2016

Member

I'm suggesting that we change it now, in both places.

This comment has been minimized.

@timmywil

timmywil Mar 28, 2016

Member

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.

@timmywil

timmywil Mar 28, 2016

Member

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.

This comment has been minimized.

@gibson042

gibson042 Mar 29, 2016

Member

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, then callbacks should be called without a this context. And although jQuery.Deferred#*With methods provide context to callbacks, we are moving in the direction of increased standards compliance, and should stop providing document context to jQuery.ready callbacks while we're already making backwards-incompatible changes to jQuery.ready.

@gibson042

gibson042 Mar 29, 2016

Member

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, then callbacks should be called without a this context. And although jQuery.Deferred#*With methods provide context to callbacks, we are moving in the direction of increased standards compliance, and should stop providing document context to jQuery.ready callbacks while we're already making backwards-incompatible changes to jQuery.ready.

This comment has been minimized.

@dmethvin

dmethvin Mar 29, 2016

Member

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.

@dmethvin

dmethvin Mar 29, 2016

Member

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.

This comment has been minimized.

@timmywil

timmywil Mar 29, 2016

Member

@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.

@timmywil

timmywil Mar 29, 2016

Member

@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.

This comment has been minimized.

@gibson042

gibson042 Mar 29, 2016

Member

Ok, I can accept that: gh-3026.

@gibson042

gibson042 Mar 29, 2016

Member

Ok, I can accept that: gh-3026.

@timmywil timmywil closed this in 5cbb234 Apr 4, 2016

@timmywil timmywil deleted the timmywil:ready-build branch Apr 4, 2016

@mgol mgol added Build and removed Needs review labels Sep 25, 2017

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