jquery / jquery/jquery-ui

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

Open
#2,300 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Behavior shared with 1.12 Comp: Widget
Dominant language
JavaScript
Stars
11.3k
Forks
5.2k
PR merge metrics
No merged PRs in 30d

Description

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:
https://github.com/jquery/jquery-ui/pull/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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at $.widget.extend and reproduce the issue with the widget example in the report, focusing on how default options arrays are initialized for successive instances. Trace the existing recursive object handling and verify that default arrays no longer accumulate changes across instances, while explicitly provided arrays retain their current behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, jquery
Domain
frontend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.