Callback isnt being called...I can't figure out why...Please help spot the bug

The book I am learning jQuery from is the sitepoint book called, “jQuery:Novice to Ninja Second Edition”. The source code for the “1-up” Notification example script which begins on page 302 of the book is the code that isn’t performing the callback. Here is the code:

$(document).ready(function(){
  $('<span>Adding</span>')
    .addClass('adding')
    .insertAfter('.wishlist');

  $('.wishlist')
    .click(function(e) {
      doOneUp(this, function() {
        $(this).prev().text('Added');
      });
      e.preventDefault();
    })
});

function doOneUp(which, callback) {
  $(which)
    .next()
    .show()
    .animate({
      top: "-=50px",
      opacity: "toggle"
    }, 1000,
    function() {
      $(this)
        .css({top: ""})
        .hide('slow', callback) // hey sitepoint users, this is the callback that isn't being called back
        .remove();
    });
}

The amount of chaining and anonymous method usage in this example of the book seems over the top. For someone learning jQuery its probably not the best code to learn from. Trying to debug 1 long line of code versus multiple smaller pieces of code seems very difficult. But all that aside I have tried debugging this code in Chrome’s debugger/inspector and have been unsuccessful. Can someone spot the bug?

It looks like the problem exists around the .remove() on the line after the callback is supposed to be called.

I made a JS Fiddle with a solution http://jsfiddle.net/GeekyJohn/KV53F/

The code below also highlights the problem.

I moved the .remove() call to the callback, so it will remove the “adding” span from there.


$(document).ready(function(){
  $('<span>Adding</span>')
    .addClass('adding')
    .insertAfter('.wishlist');

  $('.wishlist')
    .click(function(e) {
      doOneUp(this, function() {
        $(this).prev().text('Added').end().remove(); //call the remove over here.
      });
      e.preventDefault();
    })
});

function doOneUp(which, callback) {
  $(which)
    .next()
    .show()
    .animate({
      top: "-=50px",
      opacity: "toggle"
    }, 1000,
    function() {
      $(this)
        .css({top: ""})
        .hide('slow', callback);
        //.remove(); //problem is around the .remove() being called before the callback can execute
    });
}&#8203;

Hey,

Thanks for your help on this one. I’ll give it a shot soon. Do you happen to know why the code doesn’t work properly…if so, could you explain it to me what the author did wrong? Im pretty new to JavaScript and jQuery. Is the callback attached to the element that is removed from the document when .remove() is executed…in essence, removing the callback as well? I am just guessing.

I thought that all callbacks went into a global jQuery queue so that they were not technically part of any element that exists in the document. But again I am just guessing.

That’s the gist of it. Without going in to too much detail, actions are performed on an element that is being removed before the script reaches those actions. The order of which things would happen in the callback of the .animate() is:

  1. .css({top: “”}) // set the top position
  2. .hide(‘slow’, callback); //start hiding the element
  3. .remove(); // remove the element
  4. // “hide” finishes (the element has been removed from the DOM though, so the effect never gets to finish)
  5. // the callback for hide is called (but the element has been removed already, )

The reason this happens is because jQuery doesn’t wait for things like animations in the chain to finish before moving on the next item in the chain. It’s asynchronous, meaning it simply begins the execution of an item that requires a queue (like animations) and then moves on down the chain. That’s why the .remove() is best served being in the callback that finally gets executed.

I have been playing with this bit of a code a couple of hours now and I have discovered that while the jQuery .remove() method gets executed instantly, the .hide() method does not. Therefore, in my opinion the author has made this example overly complex by setting up callbacks that are unnecessary. Just for learning purposes I placed .remove() method in the callback of the .hide() method to test the functionality of the example. It appears it works just as well as any other and seems to make just a bit more sense than having to pass a callback argument to two different methods(passing the buck, or kicking the can down the road). Why put off until tomorrow what you can do today. :slight_smile:

My interpretation makes the anonymous method that’s passed in as the ‘callback’ argument to doOneUp() unnecessary at least for this small example.


function doOneUp(triggerElement, /* no longer need this parameter -> */ callback) {
				$(triggerElement).next().show(1000)
					.animate(
						{
							opacity: 0,
							top: "-=50px"
						},
						3000
					).hide('slow',
						function() {
							$(this).remove();
							triggerElement.textContent = 'Added';
							console.log('Executing callback for .hide();');
						}
					);
			}

While overly complex examples can probably help you become a better programmer in the long run, they are quite a pain in the beginning.

Almost correct :). The .hide() method gets executed before the .remove() executes, but the big difference is that .hide() causes an animation. The animation gets put in to the effects queue and won’t finish before .remove() is called, because this all happens so quickly it gives the appearance that .hide() is skipped or executed too late. (Because the animation never finishes, so the callback won’t be called either.)

(It’s worth noting that even though the original example doesn’t have a duration on the .hide(), I could not get the original code to work with or without the duration in .hide())

Im a little confused. Are you talking about the original example(the book code) or my interpretation of the solution to the problem with the original code? My solution offered in lieu of the original code works as intended as far as I can tell.

The original example from the book uses a duration of ‘slow’. I also used a duration of ‘slow’ in my solution to the original problem. I did try calling .hide() with no duration set and It seems to work fine in chrome for me. Can you post the code you are using?

I was talking about all (any?) of that used the .hide() with an animation before .remove() :slight_smile:

I have the 1st edition book, so maybe that’s something they changed in the 2nd ed.?

I’m guessing so. The author uses ‘slow’ as the first argument to the .hide() method in the example in the book.

My last gripe with this example is that the author calls the .animate() method using a key value pair of ‘opacity: “toggle”’ which effectively hides the element over the course of the animation. So calling the .hide() method directly after the .animate() method seems redundant in this example. Im not sure “toggle” was the right choice for the opacity key either since these ‘one-ups’ are meant to be a one-time deal. ‘opacity:0’ might have been a better choice.

Okay, I’ve crucified this example as much as I possibly can. Onto the next chapter in the book. :slight_smile: