Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Widget instances share a reference to default options arrays leading to unexpected behaviour #2300

Open
JoolsCaesar opened this issue Oct 1, 2024 · 3 comments

Comments

@JoolsCaesar
Copy link

JoolsCaesar commented Oct 1, 2024

Take the following example:

$.widget('test.greet', { 
   options: {names:['anonymous']}, 
   _create: function(){ 
      this.options.names.unshift('Hello');
      console.log(this.options.names.join(' '));
   } 
});
$('<div>').greet({names:['Jools']});
$('<div>').greet();
$('<div>').greet();
$('<div>').greet({names:['Adam']});
$('<div>').greet();

Expected output:

Hello Jools
Hello anonymous
Hello anonymous
Hello Adam
Hello anonymous

Actual output:

Hello Jools
Hello anonymous
Hello Hello anonymous
Hello Adam
Hello Hello Hello anonymous

Things work as expected so long as you don't rely on the default options array.

When the options object is initialised, objects are recursively recreated to ensure that they're unique to an instance. Arrays are excluded from this, which seems to be an intentional choice to preserve performance:
#193

I understand not cloning provided arrays, as they can be massive, but surely we could at least clone the default/prototype arrays. Very often an empty array is specified as a fallback or even just to clearly show what type the option is.

Obviously using an array reference that was passed in is always potentially dangerous, as you're assuming the caller is done with it, but that is a much more obvious problem than the default array being shared between all instances. Presumably this could be addressed without any significant performance hit.

I've managed to workaround this in a fairly central place as I have a base widget where I can call the following. This just recursively looks for arrays from the prototype and rebuilds them:

      _cloneOptionsArrays: function _cloneOptionsArrays(protoObj, optionObj) {
         protoObj = protoObj || this.__proto__.options;
         optionObj = optionObj || this.options;
         let key;
         for (key in optionObj) {
            const value = optionObj[key];
            const protoValue = protoObj[key];
            if ($.isPlainObject(value) && protoValue) {
               that._cloneOptionsArrays(protoValue, value);
            } else if (Array.isArray(value) && protoValue === value) {
               optionObj[key] = [].concat(value);
            }
         }
      },

I've actually only tested this in jquery ui 1.12 (sorry), but the same code ($.widget.extend) appears to be present in all later versions

@mgol
Copy link
Member

mgol commented Oct 3, 2024

Thanks for the report. Does the issue you describe exist when jQuery UI 1.12.1 is used or only with jQuery UI 1.13.0 or newer?

@mgol mgol added the Needs info label Oct 3, 2024
@JoolsCaesar
Copy link
Author

Tested with 1.12 and 1.13.3, both display the issue as described

@mgol
Copy link
Member

mgol commented Oct 4, 2024

Thanks for the info. Since the issue is already in 1.12, given limited team resources it's not likely to be fixed by the UI team; see the project status at https://blog.jqueryui.com/2021/10/jquery-maintainers-update-and-transition-jquery-ui-as-part-of-overall-modernization-efforts/. PRs are welcome if they're not too complex and contain tests.

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

No branches or pull requests

2 participants