Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for bug #12874. Tested succesfully with r.js version 2.0.2 #1028

Closed
wants to merge 1 commit into from

6 participants

@rwaldron rwaldron commented on the diff
src/callbacks.js
@@ -90,7 +90,7 @@ jQuery.Callbacks = function( options ) {
if ( list ) {
// First, we save the current length
var start = list.length;
- (function add( args ) {
+ function add( args ) {
@rwaldron Collaborator

This actually changes the semantics of the code; we can't accept this without a test that supports the change.

@timmywil Collaborator

I think he's saying the bug only happens when compiled with r.js. A qunit test may not be possible. However, if existing tests still pass, that may be all that is needed to support the change.

@rwaldron Collaborator

If that's the case, it sounds like a bug in r.js...

@rwldrn How does this change the semantics of the code?

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

Error in lint -- Please run grunt

Running "lint:dist" (lint) task
Linting dist/jquery.js...ERROR
[L1018:C21] Function declarations should not be placed in blocks. Use a function expression or move the statement to the top of the outer function.
        function add( args ) {

<WARN> Task "lint:dist" failed.� Use --force to continue. </WARN>
@eeroan

Sounds like lint don't know about hoisting. Weird. In that case the function definition should be before the call.

@dmethvin
Owner

See the discussion on the ticket.

@dmethvin dmethvin closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 2 deletions.
  1. +3 −2 src/callbacks.js
View
5 src/callbacks.js
@@ -90,7 +90,7 @@ jQuery.Callbacks = function( options ) {
if ( list ) {
// First, we save the current length
var start = list.length;
- (function add( args ) {
+ function add( args ) {
@rwaldron Collaborator

This actually changes the semantics of the code; we can't accept this without a test that supports the change.

@timmywil Collaborator

I think he's saying the bug only happens when compiled with r.js. A qunit test may not be possible. However, if existing tests still pass, that may be all that is needed to support the change.

@rwaldron Collaborator

If that's the case, it sounds like a bug in r.js...

@rwldrn How does this change the semantics of the code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
jQuery.each( args, function( _, arg ) {
var type = jQuery.type( arg );
if ( type === "function" ) {
@@ -102,7 +102,8 @@ jQuery.Callbacks = function( options ) {
add( arg );
}
});
- })( arguments );
+ }
+ add( arguments );
// Do we need to add the callbacks to the
// current firing batch?
if ( firing ) {
Something went wrong with that request. Please try again.