Compresses 6e066a4db72ff6b0d12dd8a43faec0a80e4a1fed #753

Closed
wants to merge 62 commits into
from

Conversation

Projects
None yet
@gibson042
Member

gibson042 commented Apr 26, 2012

jQuery Size - compared to last make
  253369   (+170) jquery.js
   93753     (-4) jquery.min.js
   33412     (-3) jquery.min.js.gz

rwaldron and others added some commits Mar 23, 2012

Failing test (http://gyazo.com/0a7285e1d10039bc8ec8bc340fac15a9.png)
Signed-off-by: Rick Waldron waldron.rick@gmail.com <waldron.rick@gmail.com>
Ensure innerHTML of src/dest clone nodes is correctly set. Fixes #10324
Signed-off-by: Rick Waldron waldron.rick@gmail.com <waldron.rick@gmail.com>
Makes Deferred implementation truly Promise/A compliant. Unit tests a…
…mended. Actually few changes required in jQuery's own source and we gained 8 bytes minified gzipped \o/.
$.ajax now always returns an object implementing the Promise interfac…
…e. Fixes #10944. Unit tests amended.

For back-compat, in case of an early abort, callbacks passed in the options are not called (while subsequent callbacks attached to the returned Promise are).
For early abort triggered by returning false in beforeSend, statusText is "canceled".
For much improved consistency, jqXHR.abort() sets a default statusTex…
…t of 'canceled' right until after beforeSend has been called (in which case it reverts to the default of 'abort'): now all early aborts have a statusText of 'canceled'.
Remove moot second argument from `slice.call()`
The zeroes were added to fix http://bugs.jquery.com/ticket/4942 but those browsers are no longer supported.
@SineSwiper

This comment has been minimized.

Show comment Hide comment
@SineSwiper

SineSwiper Apr 9, 2012

Hmmm, probably a better way to do it besides cssHooks, but this means that the animate side of the equation needs some re-factoring....

Hmmm, probably a better way to do it besides cssHooks, but this means that the animate side of the equation needs some re-factoring....

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 9, 2012

Member

using cssHooks allows for caching of the property so the lookup only happens once.

Member

rwaldron replied Apr 9, 2012

using cssHooks allows for caching of the property so the lookup only happens once.

@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 11, 2012

Member

Since this commit the unit tests FAIL in all browsers:

Looks like the test "Name selector non-input ([name=test])" of the selector module is failing. Duplicate name attribute.

Member

Krinkle commented on 62a4c84 Apr 11, 2012

Since this commit the unit tests FAIL in all browsers:

Looks like the test "Name selector non-input ([name=test])" of the selector module is failing. Duplicate name attribute.

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 11, 2012

Member

Weird, I'll take a look at this in AM

Member

rwaldron replied Apr 11, 2012

Weird, I'll take a look at this in AM

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 11, 2012

Member

Looks like the patch itself is not related, just the fixture

Member

rwaldron replied Apr 11, 2012

Looks like the patch itself is not related, just the fixture

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 11, 2012

Member

@dmethvin saw it happening on IRC in #jquery-dev, he started fixing it in 0f827c8. However IE7 and IE8 are still failing after that commit, different failure:

test 5: selector: element
assertion 7:    equal( jQuery("param", "#object1").length, 2, "Object/param as context" );
Expected: 2
Result: 3
Member

Krinkle replied Apr 11, 2012

@dmethvin saw it happening on IRC in #jquery-dev, he started fixing it in 0f827c8. However IE7 and IE8 are still failing after that commit, different failure:

test 5: selector: element
assertion 7:    equal( jQuery("param", "#object1").length, 2, "Object/param as context" );
Expected: 2
Result: 3

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Apr 11, 2012

Member

Looks like IE7/8 are failing on jQuery("param", "#object1") and finding the param in #object2 as well.

Member

dmethvin replied Apr 11, 2012

Looks like IE7/8 are failing on jQuery("param", "#object1") and finding the param in #object2 as well.

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 11, 2012

Member

Fixed here: #735

Member

rwaldron replied Apr 11, 2012

Fixed here: #735

@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 11, 2012

Member

This test FAILED in Opera 11.1:

test 74: event: trigger click on checkbox, fires change event
assertion 1:  ok( true, "Change event fired as a result of triggered click" );

Expected 1 assertions, but 0 were run

Either the code in general is broken in Opera (unlikely), or it needs an async guard (stop(), start())

Member

Krinkle commented on 2f1ddd4 Apr 11, 2012

This test FAILED in Opera 11.1:

test 74: event: trigger click on checkbox, fires change event
assertion 1:  ok( true, "Change event fired as a result of triggered click" );

Expected 1 assertions, but 0 were run

Either the code in general is broken in Opera (unlikely), or it needs an async guard (stop(), start())

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 16, 2012

Member

rwaldron and others added some commits Apr 11, 2012

Remove Ajax requirement for simple XML tests
Previously, all jQuery tests that wanted an XML
document would make an Ajax request to go through
jQuery's XML parsing logic in jQuery.ajax. Now,
use jQuery.parseXML instead.

This removes the need for the Ajax server for
these tests, improves their performance, and
decouples simple core tests from Ajax.

(with scottgonzalez)
@mikesherov

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Apr 16, 2012

Member

Where is this function being used?

Where is this function being used?

rwaldron added some commits Apr 16, 2012

Fixes mangled indents.
Signed-off-by: Rick Waldron waldron.rick@gmail.com <waldron.rick@gmail.com>
More indent correction
Signed-off-by: Rick Waldron waldron.rick@gmail.com <waldron.rick@gmail.com>
@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 20, 2012

Member

Introduced failure in all browsers: http://swarm.jquery.org/job/40/ ; http://swarm.jquery.org/user/jquery/

Died on test #1: createXMLDocument is not defined - { "stack": "()@./test/unit/core.js:555  ./.. }

Expected 3 assertions, but 1 were run

Introduced failure in all browsers: http://swarm.jquery.org/job/40/ ; http://swarm.jquery.org/user/jquery/

Died on test #1: createXMLDocument is not defined - { "stack": "()@./test/unit/core.js:555  ./.. }

Expected 3 assertions, but 1 were run

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 20, 2012

Member

Fixed in 16249f0

Member

Krinkle replied Apr 20, 2012

Fixed in 16249f0

@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 20, 2012

Member

Still problematic, QUnit somehow is failing to recover if this fails. Isn't the implicit stop() by asyncTest supposed to recover after a certain timeout? It's not doing that. I tried it just now with a plain test suite with just stop(), it never starts again...

Edit: Traced to 61137fd in QUnit

Member

Krinkle commented on 8fadc5b Apr 20, 2012

Still problematic, QUnit somehow is failing to recover if this fails. Isn't the implicit stop() by asyncTest supposed to recover after a certain timeout? It's not doing that. I tried it just now with a plain test suite with just stop(), it never starts again...

Edit: Traced to 61137fd in QUnit

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 20, 2012

Member

PR #746

Member

Krinkle replied Apr 20, 2012

PR #746

@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 20, 2012

Member

Was this intentional?

Was this intentional?

This comment has been minimized.

Show comment Hide comment
@markelog

markelog Apr 20, 2012

Member

No. I don't see it in my pull. I guess its rebase artefact, but Its a test, it did not break, so who cares? I'm sure it will be corrected later.

Member

markelog replied Apr 20, 2012

No. I don't see it in my pull. I guess its rebase artefact, but Its a test, it did not break, so who cares? I'm sure it will be corrected later.

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Apr 20, 2012

Member

Must have been a keyboard spasm on my part. Stray text outside elements in $() is ignored which explains why it has no effect. I will clean it up on the next commit in this area. Thanks for keeping an eye open!

Member

dmethvin replied Apr 20, 2012

Must have been a keyboard spasm on my part. Stray text outside elements in $() is ignored which explains why it has no effect. I will clean it up on the next commit in this area. Thanks for keeping an eye open!

@rwaldron

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 20, 2012

Member

I dig this :)

Member

rwaldron commented on 8ebb2f4 Apr 20, 2012

I dig this :)

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

Did you try this with var index; instead of having it as a fake parameter? Our initial variable declarations are compressed very well by gzip.

Did you try this with var index; instead of having it as a fake parameter? Our initial variable declarations are compressed very well by gzip.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

42dbc86

thanks for your expert eye.

Member

jaubourg replied Apr 25, 2012

42dbc86

thanks for your expert eye.

@mikesherov

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Apr 25, 2012

Member

We should always use real declarations. Using unused args as var declarations is hacky.

Member

mikesherov commented on 42dbc86 Apr 25, 2012

We should always use real declarations. Using unused args as var declarations is hacky.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

\o/ I'm a hacker!

Member

jaubourg replied Apr 25, 2012

\o/ I'm a hacker!

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Apr 25, 2012

Member

Sorry. I should qualify. I find it hacky. Others may disagree :)

Member

mikesherov replied Apr 25, 2012

Sorry. I should qualify. I find it hacky. Others may disagree :)

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

;P

Member

jaubourg replied Apr 25, 2012

;P

jaubourg added some commits Apr 25, 2012

Callbacks.add now accepts array-like objects (like Arguments). Now us…
…es the slice method of the args array in fireWith rather than a quite slow jQuery.merge.
@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 25, 2012

Member

This commit introduced 12 failures in the dimensions test suite in (all browsers).

Member

Krinkle commented on 58ed62e Apr 25, 2012

This commit introduced 12 failures in the dimensions test suite in (all browsers).

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 25, 2012

Member

Are those timeouts?

Member

rwaldron replied Apr 25, 2012

Are those timeouts?

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 25, 2012

Member

No, they are not. I linked to the run results and job page above. The failures are assertions, such as "Test negative width ignored (Expected: 30; Result: 0)".

Member

Krinkle replied Apr 25, 2012

No, they are not. I linked to the run results and job page above. The failures are assertions, such as "Test negative width ignored (Expected: 30; Result: 0)".

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 25, 2012

Member

I didn't have time to look at it 5 minutes ago, so I was being lazy.

Member

rwaldron replied Apr 25, 2012

I didn't have time to look at it 5 minutes ago, so I was being lazy.

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 25, 2012

Member

Hrm, I'll dig into it tonight unless @mikesherov beats me to it.

Member

gnarf replied Apr 25, 2012

Hrm, I'll dig into it tonight unless @mikesherov beats me to it.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

You may get chided for replacing all that inline code with function calls... Callbacks is one of those modules where performance might trump everything else.

Member

gibson042 commented on 97210d4 Apr 25, 2012

You may get chided for replacing all that inline code with function calls... Callbacks is one of those modules where performance might trump everything else.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

The most critical parts (initial object creation and executing callback) are untouched (if you except the initial cost of creating the options cache the first time a string is encountered).

I don't even know what got into my mind when I decided not to use any of the jQuery built-in array and iteration methods: having the duplicate code like this all over the place is asking for trouble. Beside, this has good consequences, for instance Callbacks.add accepting array-like objects and inspecting them.

Member

jaubourg replied Apr 25, 2012

The most critical parts (initial object creation and executing callback) are untouched (if you except the initial cost of creating the options cache the first time a string is encountered).

I don't even know what got into my mind when I decided not to use any of the jQuery built-in array and iteration methods: having the duplicate code like this all over the place is asking for trouble. Beside, this has good consequences, for instance Callbacks.add accepting array-like objects and inspecting them.

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 26, 2012

Member

👍 for code reuse and 👍 👍 for Callbacks.add accepting arrayish.

Member

gibson042 replied Apr 26, 2012

👍 for code reuse and 👍 👍 for Callbacks.add accepting arrayish.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

Is this method public-facing? The signature is much easier to optimize, but not as transparent.

Is this method public-facing? The signature is much easier to optimize, but not as transparent.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

Of course not, this is internal only.

Member

jaubourg replied Apr 25, 2012

Of course not, this is internal only.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

I LOVE this change.

I LOVE this change.

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 25, 2012

Member

+1 to that sentiment

Member

rwaldron replied Apr 25, 2012

+1 to that sentiment

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

Yeah, that's the big advantage of using jQuery.each in Callbacks.add!

Member

jaubourg replied Apr 25, 2012

Yeah, that's the big advantage of using jQuery.each in Callbacks.add!

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

NICE.

NICE.

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

But needs the "Deprecated" comment from above added back in.

Member

gibson042 replied Apr 25, 2012

But needs the "Deprecated" comment from above added back in.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

Not home right now but you're right. I even told myself so but completely forgot to add it back.

Member

jaubourg replied Apr 25, 2012

Not home right now but you're right. I even told myself so but completely forgot to add it back.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

Did you check the effect on gzipped size of doing it this way vs. three arguments (function, tuples[ i ^ 1 ][ 2 ].disable, tuples[ 2 ][ 2 ].lock), or is this a performance optimization?

Did you check the effect on gzipped size of doing it this way vs. three arguments (function, tuples[ i ^ 1 ][ 2 ].disable, tuples[ 2 ][ 2 ].lock), or is this a performance optimization?

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

Also, I like the unwrapping here. Great catch.

Member

gibson042 replied Apr 25, 2012

Also, I like the unwrapping here. Great catch.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

If you can make it smaller with duplicated strings, be my guess. I was more concerned with the unwrapping than I was with micro-gains here.

Member

jaubourg replied Apr 25, 2012

If you can make it smaller with duplicated strings, be my guess. I was more concerned with the unwrapping than I was with micro-gains here.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 25, 2012

Member

Ick. It's probably both smaller and clearer to put this string into the tuples like I had in earlier iterations.

Ick. It's probably both smaller and clearer to put this string into the tuples like I had in earlier iterations.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 25, 2012

Member

When I tried, it was smaller min/gzipped that way. but feel free to test and PR if it changed (I tried to change stuff one at a time, but you know how interdependant everything is when you optimize for gzip).

Member

jaubourg replied Apr 25, 2012

When I tried, it was smaller min/gzipped that way. but feel free to test and PR if it changed (I tried to change stuff one at a time, but you know how interdependant everything is when you optimize for gzip).

This comment has been minimized.

Show comment Hide comment
@staabm

staabm Apr 26, 2012

Contributor

Maybe the RE should only be compiled once?

Contributor

staabm replied Apr 26, 2012

Maybe the RE should only be compiled once?

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 26, 2012

Member

It's 20 bytes (last time I checked) against this perf hit.

Member

jaubourg replied Apr 26, 2012

It's 20 bytes (last time I checked) against this perf hit.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 26, 2012

Member

This seems a little fishy. Function.apply accepts undefined etc., so why manipulate args at all?

This seems a little fishy. Function.apply accepts undefined etc., so why manipulate args at all?

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 26, 2012

Member
Member

jaubourg replied Apr 26, 2012

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 26, 2012

Member

Hahahaha. I just knew I was putting my foot in my mouth. But still, why the slice?

Member

gibson042 replied Apr 26, 2012

Hahahaha. I just knew I was putting my foot in my mouth. But still, why the slice?

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 26, 2012

Member

Because without it, we have this:

var cb = $.Callbacks( "memory" ),
    myArray = [ "yes" ];

cb.fireWith( null, myArray );
myArray[ 0 ] = "err... no";
cb.add(function( value ) {
    console.log( value ); // outputs "err... no"
});

I removed don't protect for Arguments objects, because, quite franckly, the following deserves to fail:

(function() {
    var cb = $.Callbacks( "memory" ),
        myArray = arguments;

    cb.fireWith( null, myArray );
    myArray[ 0 ] = "err... no";
    cb.add(function( value ) {
        console.log( value ); // outputs "err... no"
    });
}

But maybe I should add it back once @rwaldron deals with these common decls (so that I don't have to declare yet another slice)

Member

jaubourg replied Apr 26, 2012

Because without it, we have this:

var cb = $.Callbacks( "memory" ),
    myArray = [ "yes" ];

cb.fireWith( null, myArray );
myArray[ 0 ] = "err... no";
cb.add(function( value ) {
    console.log( value ); // outputs "err... no"
});

I removed don't protect for Arguments objects, because, quite franckly, the following deserves to fail:

(function() {
    var cb = $.Callbacks( "memory" ),
        myArray = arguments;

    cb.fireWith( null, myArray );
    myArray[ 0 ] = "err... no";
    cb.add(function( value ) {
        console.log( value ); // outputs "err... no"
    });
}

But maybe I should add it back once @rwaldron deals with these common decls (so that I don't have to declare yet another slice)

This comment has been minimized.

Show comment Hide comment
@staabm

staabm Apr 26, 2012

Contributor

@rwldrn

Contributor

staabm replied Apr 26, 2012

@rwldrn

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 26, 2012

Member

Damn him and his silly username! ;)

Member

jaubourg replied Apr 26, 2012

Damn him and his silly username! ;)

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 26, 2012

Member

You're good. That addresses both my primary question and my now unnecessary follow-up, but I'm not convinced that we should treat arrays and arguments differently. I'm with you on the common declarations; stack.push( slice.call( args || [] ) ) is definitely where we want to go in the long run (and that is acceptable because IE throws an exception on anything that isn't array/arguments... so give us a length and we'll take a list 😉).

Member

gibson042 replied Apr 26, 2012

You're good. That addresses both my primary question and my now unnecessary follow-up, but I'm not convinced that we should treat arrays and arguments differently. I'm with you on the common declarations; stack.push( slice.call( args || [] ) ) is definitely where we want to go in the long run (and that is acceptable because IE throws an exception on anything that isn't array/arguments... so give us a length and we'll take a list 😉).

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 26, 2012

Member

Yeah, sorry about the username non-sense. I registered as "rwldrn" a long time ago. I also have "rwaldron" but that doesn't have any of my project stuff :\

Member

rwaldron replied Apr 26, 2012

Yeah, sorry about the username non-sense. I registered as "rwldrn" a long time ago. I also have "rwaldron" but that doesn't have any of my project stuff :\

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Apr 26, 2012

Member

@rwldrn yeah, I find you a little short on this one... Bwahahahahaha! ;P

Member

jaubourg replied Apr 26, 2012

@rwldrn yeah, I find you a little short on this one... Bwahahahahaha! ;P

@gibson042 gibson042 closed this Apr 26, 2012

@davidmurdoch

This comment has been minimized.

Show comment Hide comment
@davidmurdoch

davidmurdoch May 16, 2012

Contributor

OMG A TYPO!

Contributor

davidmurdoch commented on src/ajax/jsonp.js in b0ea80e May 16, 2012

OMG A TYPO!

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin May 17, 2012

Member

This comment has been minimized.

Show comment Hide comment
@davidmurdoch

davidmurdoch May 17, 2012

Contributor

(facepalm)

Let's just pretend I was never here, k?

Contributor

davidmurdoch replied May 17, 2012

(facepalm)

Let's just pretend I was never here, k?

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil May 17, 2012

Member

I've never seen that before either @davidmurdoch.

Member

timmywil replied May 17, 2012

I've never seen that before either @davidmurdoch.

@briancavalier

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Jun 12, 2012

Cool, it's great to see this headed toward Promises/A! I think it's gonna become the norm for multiple promise implementations to be in play in any reasonably interesting app. I found a situation where I believe it still isn't fully compliant, so I created a quick test case. It should show 3 for both jQuery and when.js, but the jQuery.Deferred seems never to resolve.

Cool, it's great to see this headed toward Promises/A! I think it's gonna become the norm for multiple promise implementations to be in play in any reasonably interesting app. I found a situation where I believe it still isn't fully compliant, so I created a quick test case. It should show 3 for both jQuery and when.js, but the jQuery.Deferred seems never to resolve.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Jun 13, 2012

Member

It seems to me that when.js wrongly calls the last resolve callback. I'd expect this to be the correct code and behaviour.

Member

jaubourg replied Jun 13, 2012

It seems to me that when.js wrongly calls the last resolve callback. I'd expect this to be the correct code and behaviour.

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Jun 13, 2012

Hey @jaubourg,

I'm pretty sure the correct behavior is that the promise chain should return to resolving when a registered rejection handler does not explicitly propagate the rejection by either throwing or returning a rejected promise--i.e. it should behave similar to a catch statement. To propagate the exception, a catch statement must explicitly rethrow (or throw a new exception). If it doesn't explicitly (re)throw, the original exception is considered to be handled and doesn't propagate.

I've updated the original fiddle to include Q, which matches when.js's behavior. I also created a new fiddle that includes when.js, Q, and dojo (1.6, because it's easier to use in jsfiddle than 1.7, but the behavior is the same). Those show the same behavior, so I think jQuery is the outlier in this case.

Please don't take this the wrong way. Like I said, I'm really interested in trying to ensure that the most popular promise implementations are all interoperable, and hopefully this is a test case that will help.

Hey @jaubourg,

I'm pretty sure the correct behavior is that the promise chain should return to resolving when a registered rejection handler does not explicitly propagate the rejection by either throwing or returning a rejected promise--i.e. it should behave similar to a catch statement. To propagate the exception, a catch statement must explicitly rethrow (or throw a new exception). If it doesn't explicitly (re)throw, the original exception is considered to be handled and doesn't propagate.

I've updated the original fiddle to include Q, which matches when.js's behavior. I also created a new fiddle that includes when.js, Q, and dojo (1.6, because it's easier to use in jsfiddle than 1.7, but the behavior is the same). Those show the same behavior, so I think jQuery is the outlier in this case.

Please don't take this the wrong way. Like I said, I'm really interested in trying to ensure that the most popular promise implementations are all interoperable, and hopefully this is a test case that will help.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Jun 13, 2012

Member

Well, Brian, it's probably the first time I see jQuery being called the "outlier" rather than the nasty-hype-machine-that-will-devour-the-world (tm), so it's quite refreshing ;)

This use-case is probably yet another nail in the coffin of jQuery's Promise/A compliance. You probably followed the multiple discussions that took place about this. Equating exceptions and rejections is something we cannot do in a our environment: it makes debugging impossible. The metaphor is broken, btw, because, with Promise/A, exceptions are always handled (which is the gist of the problem). But let's not beat a dead horse.

Here is why I have a problem with rejection handlers changing state:

// Promise/A
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => resolved
promise.then( null, null, function() {}  ); // => pending

// jQuery
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => rejected
promise.then( null, null, function() {}  ); // => pending

"Redirecting" state has to be done explicitly in jQuery's implementation: it's explicit and it's symmetrical.

If people want us to write down a spec of jQuery's take on Promises, I'll try and find time to do so (though I'd love to get some help on this one). However, pushing jQuery's implementation forward to be fully Promise/A compliant is probably never gonna happen at this point. As minimal as Promise/A appeared to be, it has one contraint too many, at least for us.

Member

jaubourg replied Jun 13, 2012

Well, Brian, it's probably the first time I see jQuery being called the "outlier" rather than the nasty-hype-machine-that-will-devour-the-world (tm), so it's quite refreshing ;)

This use-case is probably yet another nail in the coffin of jQuery's Promise/A compliance. You probably followed the multiple discussions that took place about this. Equating exceptions and rejections is something we cannot do in a our environment: it makes debugging impossible. The metaphor is broken, btw, because, with Promise/A, exceptions are always handled (which is the gist of the problem). But let's not beat a dead horse.

Here is why I have a problem with rejection handlers changing state:

// Promise/A
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => resolved
promise.then( null, null, function() {}  ); // => pending

// jQuery
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => rejected
promise.then( null, null, function() {}  ); // => pending

"Redirecting" state has to be done explicitly in jQuery's implementation: it's explicit and it's symmetrical.

If people want us to write down a spec of jQuery's take on Promises, I'll try and find time to do so (though I'd love to get some help on this one). However, pushing jQuery's implementation forward to be fully Promise/A compliant is probably never gonna happen at this point. As minimal as Promise/A appeared to be, it has one contraint too many, at least for us.

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Jun 14, 2012

I'm def not here to be a jQuery-hater, just trying to ensure some interoperability.

I disagree with your feelings about Promises/A, and I don't see Promises/A as having too many constraints at all. In fact, I see it as quite simple:

  1. If a handler returns successfully, the next promise is resolved.
  2. If a handler throws or returns a rejection, the next promise is rejected.

That said, I respect that every project has its own goals and constraints. If it doesn't make sense for jQuery to provide a Promises/A compliant promise, then I do think it's important to document its promise behavior.

Given the current (good) trend of more modular code, and moving more responsiblity (and therefore, code) to the browser-side, it's inevitable that applications will contain multiple promise implementations. Having an equalizer like when.js's when(), that can consume all of the above will help.

I think it would make sense for jQuery documentation to point people to when.js or Q as ways to enable promise interoperability.

I'm def not here to be a jQuery-hater, just trying to ensure some interoperability.

I disagree with your feelings about Promises/A, and I don't see Promises/A as having too many constraints at all. In fact, I see it as quite simple:

  1. If a handler returns successfully, the next promise is resolved.
  2. If a handler throws or returns a rejection, the next promise is rejected.

That said, I respect that every project has its own goals and constraints. If it doesn't make sense for jQuery to provide a Promises/A compliant promise, then I do think it's important to document its promise behavior.

Given the current (good) trend of more modular code, and moving more responsiblity (and therefore, code) to the browser-side, it's inevitable that applications will contain multiple promise implementations. Having an equalizer like when.js's when(), that can consume all of the above will help.

I think it would make sense for jQuery documentation to point people to when.js or Q as ways to enable promise interoperability.

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Jun 15, 2012

I'll add that from my perspective, @dmethvin's question is one of application design, and not of promises or exceptions. The data used as an error payload is, imho, separate from the mechanism that transports the error. Rejecting a promise with custom data is analogous to throwing that custom data. Same data, different transport. If your application design is one that uses custom data in that way, then, as @domenic described, Promises/A simply gives you an asynchronous transport that is analogous to the synchronous transport provided by throw. Any application-level logic built around the data would remain nearly identical when using Promises/A, as Domenic's gist shows.

I'll add that from my perspective, @dmethvin's question is one of application design, and not of promises or exceptions. The data used as an error payload is, imho, separate from the mechanism that transports the error. Rejecting a promise with custom data is analogous to throwing that custom data. Same data, different transport. If your application design is one that uses custom data in that way, then, as @domenic described, Promises/A simply gives you an asynchronous transport that is analogous to the synchronous transport provided by throw. Any application-level logic built around the data would remain nearly identical when using Promises/A, as Domenic's gist shows.

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Jun 15, 2012

@scottgonzalez it depends entirely on what was thrown. You can throw anything, and a catch statement higher up will receive it verbatim:

node -e 'try { throw "hello world"; } catch(message) { console.log(message, typeof message); }'

I'm certainly not advocating that in application code ;)

@scottgonzalez it depends entirely on what was thrown. You can throw anything, and a catch statement higher up will receive it verbatim:

node -e 'try { throw "hello world"; } catch(message) { console.log(message, typeof message); }'

I'm certainly not advocating that in application code ;)

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Jun 15, 2012

Member

Let me try to make this more concrete, so I can tell whether this is a misunderstanding on my part or yours of the impact this would have to existing jQuery code.

jQuery wears several hats in this conversation: 1) An implementer of something that aspires to be Promise/A compliant. 2) A client of the current $.Deferred implementation, for example in $.ajax. and $.fn.animate. 3) A provider of the current $.Deferred implementation to jQuery developers.

Let's say someone has included jQuery in their web page and writes this code:

$.ajax(url).then(
    function(jqXHR) { /* do successful stuff */ },
    function(jqXHR, errorCode) { /* deal with failed request */ }
);

If some code or callback has a code error today, the browser throws an uncaught error with all sorts of detail which is visible on the console. It can also be caught by a window.onerror handler and sent back to a server for analysis using tools such as DamnIT, Errorception, AirBrake, Runtime Intelligence, or Google Analytics. For unanticipated script errors that fall into the "Never check for an error condition you don't know how to handle" category, this is by far the lowest-hassle way to go.

The proposal on the table as I understand it is to catch the error and pass it to the fail handler, which is expecting the (jqXHR, errorCode) it always got in the past but now has to deal with (Error) as well. The same goes for a developer's direct use of jQuery $.Deferred. So to handle a case that is supposedly a "de-facto standard" (of the Promise/A proposal which is not a standard) we should put on Steve's hat and tell everyone to go back and rewrite their code to accommodate this? Plus, it's again asking the developer to handle a totally unanticipated condition, and giving them less information than if they had let it get to the window.onerror handler or the browser's error console.

I have been talking with @jaubourg about how we might be able to change the semantics with a flag to make it do this, but even so this seems like it would have to be opt-in or we'd be breaking code all over the place. And to prevent further hassles, we should be recommending that all fail handlers take a single Error or Error-like argument unless the function is willing to sniff around (which seems incredibly ugly to me). Still, as large as the jQuery ecosystem is, there would be a mix of old-behavior and new-behavior code out there for years to come.

Is it clear now why we can't just change the fundamental behavior of our promise implementation? Or is there some easy way out? If the issue is simply that we're not really Promise/A and can't become so without breaking user code, one option would simply be to publicize that our promises implementation isn't interoperable and explain why we placed user code compatibility over interoperability.

Member

dmethvin replied Jun 15, 2012

Let me try to make this more concrete, so I can tell whether this is a misunderstanding on my part or yours of the impact this would have to existing jQuery code.

jQuery wears several hats in this conversation: 1) An implementer of something that aspires to be Promise/A compliant. 2) A client of the current $.Deferred implementation, for example in $.ajax. and $.fn.animate. 3) A provider of the current $.Deferred implementation to jQuery developers.

Let's say someone has included jQuery in their web page and writes this code:

$.ajax(url).then(
    function(jqXHR) { /* do successful stuff */ },
    function(jqXHR, errorCode) { /* deal with failed request */ }
);

If some code or callback has a code error today, the browser throws an uncaught error with all sorts of detail which is visible on the console. It can also be caught by a window.onerror handler and sent back to a server for analysis using tools such as DamnIT, Errorception, AirBrake, Runtime Intelligence, or Google Analytics. For unanticipated script errors that fall into the "Never check for an error condition you don't know how to handle" category, this is by far the lowest-hassle way to go.

The proposal on the table as I understand it is to catch the error and pass it to the fail handler, which is expecting the (jqXHR, errorCode) it always got in the past but now has to deal with (Error) as well. The same goes for a developer's direct use of jQuery $.Deferred. So to handle a case that is supposedly a "de-facto standard" (of the Promise/A proposal which is not a standard) we should put on Steve's hat and tell everyone to go back and rewrite their code to accommodate this? Plus, it's again asking the developer to handle a totally unanticipated condition, and giving them less information than if they had let it get to the window.onerror handler or the browser's error console.

I have been talking with @jaubourg about how we might be able to change the semantics with a flag to make it do this, but even so this seems like it would have to be opt-in or we'd be breaking code all over the place. And to prevent further hassles, we should be recommending that all fail handlers take a single Error or Error-like argument unless the function is willing to sniff around (which seems incredibly ugly to me). Still, as large as the jQuery ecosystem is, there would be a mix of old-behavior and new-behavior code out there for years to come.

Is it clear now why we can't just change the fundamental behavior of our promise implementation? Or is there some easy way out? If the issue is simply that we're not really Promise/A and can't become so without breaking user code, one option would simply be to publicize that our promises implementation isn't interoperable and explain why we placed user code compatibility over interoperability.

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Jun 15, 2012

Ok, I think it's time step back for a moment and remember how this thread started by rereading the commit message and my initial comment.

The stated intent of the commit was for jQuery Deferred to be made Promises/A compliant. I came upon it, and was very happy to see this. In an effort to try to help, I supplied a test case where post-this-commit, it isn't compliant. I think it's also important to note that both @domenic and I have said that we respect that jQuery has it's own goals and priorities, like every project. Neither of us has in any way demanded that jQuery Deferred become Promises/A. The intent to do so was implied by the commit.

If Promises/A is not a desirable goal of jQuery Deferred, I respect that. Unfortunately, I believe there is pain for developers either way.

My original suggestion was for jQuery documentation to point developers who need or desire promise interoperability to when.js or Q, as their when() functions can provide it.

And @jaubourg suggested documenting jQuery's promise behavior.

I stand by both of those as reasonable suggestions that would benefit jQuery, implementors of other libraries, and the larger Javascript developer community.

Ok, I think it's time step back for a moment and remember how this thread started by rereading the commit message and my initial comment.

The stated intent of the commit was for jQuery Deferred to be made Promises/A compliant. I came upon it, and was very happy to see this. In an effort to try to help, I supplied a test case where post-this-commit, it isn't compliant. I think it's also important to note that both @domenic and I have said that we respect that jQuery has it's own goals and priorities, like every project. Neither of us has in any way demanded that jQuery Deferred become Promises/A. The intent to do so was implied by the commit.

If Promises/A is not a desirable goal of jQuery Deferred, I respect that. Unfortunately, I believe there is pain for developers either way.

My original suggestion was for jQuery documentation to point developers who need or desire promise interoperability to when.js or Q, as their when() functions can provide it.

And @jaubourg suggested documenting jQuery's promise behavior.

I stand by both of those as reasonable suggestions that would benefit jQuery, implementors of other libraries, and the larger Javascript developer community.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Jun 15, 2012

@briancavalier basically said everything I would want to say. I just jumped in to help educate and clarify, not to recommend Promises/A interoperation---I assumed that battle was already lost. As you say, @dmethvin, there's really no easy way out.

The only thing I would argue for, that nobody else seems to be behind, is dropping then completely so that jQuery properly fails the Promises/A duck-type test. This makes a lot of sense to me: as of 1.7, it's just a less powerful pipe, and most examples use and encourage done anyway. So perhaps deprecate in 1.8, remove in 1.9. But it's not that hard to deal with anyway, so whatevs.

@briancavalier basically said everything I would want to say. I just jumped in to help educate and clarify, not to recommend Promises/A interoperation---I assumed that battle was already lost. As you say, @dmethvin, there's really no easy way out.

The only thing I would argue for, that nobody else seems to be behind, is dropping then completely so that jQuery properly fails the Promises/A duck-type test. This makes a lot of sense to me: as of 1.7, it's just a less powerful pipe, and most examples use and encourage done anyway. So perhaps deprecate in 1.8, remove in 1.9. But it's not that hard to deal with anyway, so whatevs.

@krisselden

This comment has been minimized.

Show comment Hide comment
@krisselden

krisselden Oct 6, 2012

This treats every Function as an array like object (and String too but that was fixed later) and loops through prototype, so if something like say Modernizr pollyfills bind, it will add bind to the callbacks.

This treats every Function as an array like object (and String too but that was fixed later) and loops through prototype, so if something like say Modernizr pollyfills bind, it will add bind to the callbacks.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Oct 6, 2012

Member

Except no function is ever passed to this test. Just look at #97.

Also, jQuery doesn't support Object.prototype extensions.

Member

jaubourg replied Oct 6, 2012

Except no function is ever passed to this test. Just look at #97.

Also, jQuery doesn't support Object.prototype extensions.

This comment has been minimized.

Show comment Hide comment
@krisselden

krisselden Oct 6, 2012

That is simply not true ( !options.unique || !self.has( arg ) ) can fall through to else if with a function: http://jsfiddle.net/krisselden/JFSNW/

Modernizr bind is a Function.prototype extension not an Object.prototype extension and it doesn't require it being on the prototype to make it fail (see above fiddle).

The intention of the unique check I'm sure is just to prevent list.push( arg ); not to recursively add, the meaning was changed when the condition was reordered.

That is simply not true ( !options.unique || !self.has( arg ) ) can fall through to else if with a function: http://jsfiddle.net/krisselden/JFSNW/

Modernizr bind is a Function.prototype extension not an Object.prototype extension and it doesn't require it being on the prototype to make it fail (see above fiddle).

The intention of the unique check I'm sure is just to prevent list.push( arg ); not to recursively add, the meaning was changed when the condition was reordered.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Oct 6, 2012

Member

Very nice catch! Could you open a ticket for that in the bug tracker so that we can tackle this properly (easy given you gave us the unit test \o/).

Member

jaubourg replied Oct 6, 2012

Very nice catch! Could you open a ticket for that in the bug tracker so that we can tackle this properly (easy given you gave us the unit test \o/).

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Oct 6, 2012

Member

Nevermind, I did it already: http://bugs.jquery.com/ticket/12665

Member

jaubourg replied Oct 6, 2012

Nevermind, I did it already: http://bugs.jquery.com/ticket/12665

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Oct 6, 2012

Member
Member

jaubourg replied Oct 6, 2012

@chee

This comment has been minimized.

Show comment Hide comment
@chee

chee Oct 16, 2012

thank-you <3

chee commented on d7217cc Oct 16, 2012

thank-you <3

@yapp-ci

This comment has been minimized.

Show comment Hide comment
@yapp-ci

yapp-ci Nov 6, 2012

The || 0 in this line causes a crash in Android browser 2.3.x earlier than 2.3.6 See http://bugs.jquery.com/ticket/12497

I would like to provide a fix, but am unsure of the intent of the || 0. Can you explain?

-@lukemelia

The || 0 in this line causes a crash in Android browser 2.3.x earlier than 2.3.6 See http://bugs.jquery.com/ticket/12497

I would like to provide a fix, but am unsure of the intent of the || 0. Can you explain?

-@lukemelia

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