6

I have searched around for this but thus far have not been able to find a duplicate, I may be using the wrong keywords...

I am trying to temporarily change a function stored in an object, but am having trouble setting it back to what it was before.

Consider this:

// Set the options object
var options = {
    success: function(){
        console.log('Original Function Called');
    }
}

// Save the options
$('#foo').data('bar',options);

And then in another function:

// Get the options
var options = $('#foo').data('bar');

// Store the old options
var old_options = options;

// Temporarily change the success function
options.success = function(){
    console.log('Temporary Function Called');
}

// Save the options
// This allows the other functions to access the temporary function
$('#foo').data('bar',options);

// Do stuff here that uses the new options

// Reset the options to include the original success function
$('#foo').data('bar',old_options);

I would have expected that to only display Temporary Function Called once, however, it seems to completely replace the old success callback with the temporary callback.

Can anyone tell me why and how I can get around this?

UPDATE

I thought that extend would fix this but it seems that the issue may be a little deeper. I have decided to post a snippet of my actual code this time. Please be aware of the following before reading:

  1. SM is pretty much just an alias of jQuery, please ignore it.
  2. success and error are parameters supplied to the function

Here is my code:

// Get the properties
var properties = $(form).data('autosave');
switch(parameter){
    case 'save':
        var old_properties = $.extend({},properties);
        // Set the new callbacks if they have been supplied
        properties.options.success = typeof success!=='undefined' ? success : old_properties.options.success;
        properties.options.error = typeof error!=='undefined' ? error : old_properties.options.error;
        // Save the properties
        $(form).data('autosave',properties);
        // Call the save method before setting the interval
        SM(form)._as_save();
            properties = $.extend({},old_properties);
        // Save the old properties
        $(form).data('autosave',properties);
        // Clear the current interval
        clearInterval(properties.interval);
        // Call the save method periodically
        properties.interval = setInterval(function(){
        SM(form)._as_save();
        },properties.options.interval);
    break;
}
// Save the properties
$(form).data('autosave',properties);
Ben Carey
  • 16,540
  • 19
  • 87
  • 169
  • You're saying that after `$('#foo').data('bar',old_options);` you are still seeing `Temporary Function Called`? – crush Jun 19 '13 at 20:54
  • Some good answers down there. – crush Jun 19 '13 at 20:56
  • I'm a bit confused - this new update, was that after our discussion of the shallow copy vs. deep copy? I see your `success` method is actually an `options.success()` method (and similarly for `error`). That is exactly the situation where a deep copy would be required. So are things still broken or did the deep copy do the trick? – Michael Geary Jun 20 '13 at 02:42
  • @MichaelGeary No this came before :-) The deep fixed it – Ben Carey Jun 20 '13 at 09:11
  • Whew! You had me worried. It makes sense now, looking at your code in the update: The shallow copy would have worked with your code in the original question, where the `success` callback was a first-level property of the main options object. But the actual code in your update is a bit different: it has a `properties.options.success` function - not a direct child object but two levels down. That's exactly the situation where a deep copy is needed (or at least a more specialized copy that would handle this nested object). – Michael Geary Jun 20 '13 at 09:42

4 Answers4

10

When you run this code:

var old_options = options;

you are not making a copy of the entire options object that you can restore later. You are merely saving a reference to the same object. In other words, old_options is the very same object as options, so when you assign a new value into options.success, you're changing it in both options and old_options—because they are the same object.

To fix this, you can use an object cloning function to make a copy of the object which you can then restore later. Since you're using jQuery, you can change the line above to:

var old_options = $.extend( true, {}, options );

Now, when you change options.success, you're only changing it in the options object. old_options is unaffected, so your later call will restore it successfully:

$('#foo').data('bar',old_options);

Interestingly enough, this may still work OK even if options.success is an asynchronous callback (which sounds likely from the name). That's because whatever code calls that .success() method later on, they should still be holding on to a reference to your modified options object—even if you've restored the old one back into the element's data in the meantime. At least one could hope for that; if the other code digs back into the $().data() to find the .success callback then you'd be in trouble.

The $.extend() call above does a "deep" (recursive) copy of the options object. That is, if one of the properties inside options is itself an object, it also clones that object instead of just copying a reference to it.

If you leave out the true argument, $.extend() does a shallow copy instead:

var old_options = $.extend( {}, options );

This still creates a new object and copies over all the properties from an existing object, but if one of those properties is itself an object it doesn't clone that object, it just copies a reference. This is more efficient if it works with the structure of the object you're using, otherwise you can use the deep copy.

If the properties/methods you need to save and restore are direct children of the main object, a shallow copy should be enough. Here's a case where you'd definitely need a deep copy:

{
    url: 'test',
    events: {
        success: function( data ) {
            // ...
        }
    }
}

Here we have an object with an events property, and that property is itself an object with some properties/methods of its own (in this example an events.success() method. If you do a shallow copy of this object, the original and the copy will share a common events object. So if you did something like this:

options.events.success = function(...) {...};

You'd actually be updating that in both options and old_options. No good. That's where a deep copy is needed.

Michael Geary
  • 28,450
  • 9
  • 65
  • 75
  • That is what I thought I might be doing. Should I just use the jQuery `extend` method? – Ben Carey Jun 19 '13 at 20:55
  • Perfect, thank you!! Wasnt too sure how to word the question but I thought this might be the reason. Thank you for your time – Ben Carey Jun 19 '13 at 20:57
  • Its a little more complicated than that, thats why I didn't post my code. Think of it as a jQuery extension (it is more than this but not really relevant). If you call `$('#foo').bar(options)` it creates and instance of the bar. And then you can call `$('#foo').bar('save',function(){})` which execute the save method and call the callback supplied just once and then revert back to the callback specified in the original instance `options` – Ben Carey Jun 19 '13 at 21:04
  • It's funny, but I was adding that line of code with `$.extend()` at the very moment you were writing that comment asking about it! Plus of course all the other answers saying the same thing. I guess the old saying is at least sometimes true: great minds think alike. :-) – Michael Geary Jun 19 '13 at 21:04
  • Hahaha. Irritating thing is that I was going to try `extend()` but didn't think it would have made a difference... Would have deleted this post as soon as I saw your answer, but as you got such a good load of rep out of it thought I would leave it :-) – Ben Carey Jun 19 '13 at 21:06
  • Okeydoke, well the real question is what order these things happen in: who picks up a reference to that `success` function and when do they save that reference for their own (later) use. Maybe you're OK, but as you can imagine when I hear "saving and restoring object/function references" and "success callback" (which sounds asynchronous) in the same breath I start to worry. :-) – Michael Geary Jun 19 '13 at 21:09
  • "as you got such a good load of rep out of it thought I would leave it" - Ah, that is funny, and very kind of you! Perhaps more important is whether it will be useful to future visitors - and hopefully it will be helpful to someone. – Michael Geary Jun 19 '13 at 21:11
  • Wouldn't it be better automatically add a callback to the `complete` function that will execute and restore old settings? The example seems like you want it to happen regardless. – Justin Jun 19 '13 at 21:13
  • @Justin Possibly, I may do this if I see it as necessary. – Ben Carey Jun 19 '13 at 21:15
  • @MichaelGeary It is quite hard to explain the whole process here as it is a pretty large script, and it is used in many different ways... I think it is sorted though. If I haev any issues, I will come back. Thanks so much for your help :-) – Ben Carey Jun 19 '13 at 21:16
  • @MichaelGeary I spoke too soon, turns out `extend` did not fix it... take a look at my update to see what I am actually doing. It may be that I am missing something obvious... – Ben Carey Jun 19 '13 at 21:36
  • @MichaelGeary I think your latest update has hit the nail on the head. I need a deep copy as options is actually a object within properties. However, jQuery offers the `deep` parameter for this, so surely this would sort it? It didnt when I tried... – Ben Carey Jun 19 '13 at 21:41
  • @MichaelGeary Sorted!!! I was calling `$.extend(true,old_properties);` not `$.extend(true,{},old_properties);`... Thanks again! – Ben Carey Jun 19 '13 at 21:44
  • Ah yes, I forgot about the `deep` option to `$.extend()`, thanks for the reminder. I'm surprised that was needed, though - unless there were other nested objects? But in any case if you have it working now, that is cool! There's no real drawback to using the deep copy here even if it weren't necessary. – Michael Geary Jun 19 '13 at 21:52
  • And thanks for reporting back on your progress and solutions. In the spirit of making this useful for future visitors, I updated the answer to include your findings. – Michael Geary Jun 19 '13 at 22:03
3

The problem is that options has reference semantics:

// Store the old options
var old_options = options;

This comment lies. You do not have a copy of the old options; rather, you have another reference to the same options object (another name with which you can refer to it).

So when you overwrite options.success, this change is also visible on old_options. The code that uses .data to store and revert the value of the options is redundant.

The only thing you need to do is this:

var old_success = options.success;
options.success = function() { /* whatever */ };

// code that uses the new success callback

options.success = old_success; // restore original value
Jon
  • 428,835
  • 81
  • 738
  • 806
  • @MichaelGeary has got it, but just one comment on your answer. I originally tried this method but it ended up executing `options.success`... – Ben Carey Jun 19 '13 at 20:59
  • Even if the solution turned out to be different, you get a hearty +1 for this note: **"This comment lies."** That is such an important point: oftentimes a bug is caused because we think the code is doing what a comment says it does - whether that's an explicit comment or a mental "comment" - i.e. our [mis]understanding of the code. It's all too easy to overlook a case where we *think* the code is doing one thing but it really does another. I've done that many times, that's how I know :-) – Michael Geary Jun 20 '13 at 02:33
2

Your issue is you are working with objects. You are passing around references to the object. The underlying object is the same, all you have done is add a variable that points at the same address.

You will have to clone the object. EG create a new object with the same properties and copy them over.

This post may be useful to you.

Community
  • 1
  • 1
Justin
  • 3,337
  • 3
  • 16
  • 27
2

When you do this var old_options = options you're not copying options values but a reference to it. From now on, old_options and options point to the same memory spot and changes on any of them affect to both.

Claudio Redi
  • 67,454
  • 15
  • 130
  • 155