Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8645 closed bug (wontfix)

Array option in widget isn't cloned

Reported by: warsky Owned by:
Priority: minor Milestone: 1.9.1
Component: ui.widget Version: 1.9.0
Keywords: Cc:
Blocked by: Blocking:

Description

We built a number of jQuery widgets under 1.8. Many of these widgets had options that were arrays:

options :

myArrayOption : [1, 2, 3]

and this worked well. Not so under 1.9. Under 1.9, when the widget is instantiated, instead of making a copy of the array option, it makes a copy of the reference to the array. So, if you instantiate the widget multiple times, they all reference the same array and changing one changes them all. It appears the problem is caused because 1.9.0 uses the new $.widget.extend() instead of the regular jquery.extend() The $.widget form does not handle arrays properly.

The fix is simple, in $.widget.extend() change:

target[ key ] = $.isPlainObject( value ) ? $.widget.extend( {}, target[ key ], value ) : value;

to:

if ($.isArray(value)) {

target[key] = $.widget.extend([], target[ key ], value);

} else {

target[ key ] = $.isPlainObject( value ) ? $.widget.extend( {}, target[ key ], value ) : value;

}

I have worked around it by simply overriding $.widget.extend, which also requires changing the first line from slice.call(...) to Array.prototype.slice.call(...)

I'm going to let someone else change make the change to the repository...

Change History (9)

comment:1 Changed 7 years ago by Scott González

Resolution: wontfix
Status: newclosed

This is intentional. It's impossible to keep a reference if we do a copy, but it's easy for you to copy if that's what you want. The old behavior was very problematic.

comment:2 Changed 7 years ago by warsky

OK, but in JavaScript an array is essentially just an object where you use indices to reference elements instead of names. Why wouldn't you want the behavior to be the same for arrays and objects? You copy objects, but not arrays.

Perhaps there is some place in the framework where this causes trouble that I don't know about but I definitely don't see why when you instantiate widgets of the same type with an option that is an array that you would want all those widgets to reference the same array. Sure, I can create a routine that does a copy, but that means two steps for instantiation, and means all existing code has to be changed to the two step instantiation process.

comment:3 Changed 7 years ago by Scott González

There are a lot of cases where arrays are data sources and being able to modify the original and have it change the widget is important.

It's not exactly a two step process, it's just the addition of a .each()

elements.each(function() {
    $( this ).widget({ option: array.slice() });
});

Alternatively, you can change your widgets to do clones internally on create.

Last edited 7 years ago by Scott González (previous) (diff)

comment:4 Changed 7 years ago by richard.s.hall

Personally, I agree with the original poster. It seems somewhat odd that the behavior would be different between the two. Although, I slightly disagree with the poster in that I think this behavior doesn't make sense at all and it should always be pass by reference for all options. It was painful for me to learn that it wasn't this way, since it makes the most sense to me.

Can someone comment on why cloning happens in the first place and also on why it makes more sense to have different behavior based on argument type than being consistent?

I think it makes the most sense to always pass by reference regardless of the argument type, because, as Scott points out, it is easy to copy if that's what you want, but much more painful to keep references if it is copied by default.

comment:5 Changed 7 years ago by Scott González

Richard: It's because pass by reference is a huge surprise to almost everyone. People do multipleElements.widget( options ) all the time.

comment:6 in reply to:  5 Changed 7 years ago by richard.s.hall

Replying to scott.gonzalez:

Richard: It's because pass by reference is a huge surprise to almost everyone. People do multipleElements.widget( options ) all the time.

Yeah, I guess so.

However, it is still not really clear to me why this would be less surprising in the array case than in the object case, especially enough to justify an inconsistency in the API.

Seems like it would be better for you to provide some sort of mechanism for us to say explicitly what we want to have happen, rather than having you guys guess what we want.

Thanks for the response.

comment:7 Changed 7 years ago by rogeriorc

http://jsfiddle.net/T4NvA/5/

Here's a example. I instantiate 4 distinct Widgets, the ones that I don't pass the array parameters share the same reference.

comment:8 Changed 7 years ago by Scott González

You shouldn't define an array as the default value. You should be setting an empty array if no rows are passed on create.

comment:9 Changed 7 years ago by warsky

OK, so basically you are saying that default values should be set in create, rather than in the options declaration? That should work, but certainly makes it less clean unless you set all the options in create.

My biggest problem is that there was a rather fundamental change made to a rather established and stable API that addresses limited use cases and that breaks existing software in a rather significant way that is not always readily apparent (i.e. it doesn't just create a javascript error). Browser based developers have enough troubles keeping up with subtle browser changes. Hell, much of the reason for using an API like jQuery is that it hides these browser differences and changes and let's us program against a consistent API. But this change shows that maybe the goal of jQuery isn't to be consistent; that I have to worry about some subtle break every time I upgrade jQuery.

Note: See TracTickets for help on using tickets.