Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#9472 closed bug (notabug)

deep copy of options in _createWidget is problematic

Reported by: rattigan Owned by: rattigan
Priority: minor Milestone: none
Component: ui.widget Version: 1.10.3
Keywords: Cc:
Blocked by: Blocking:

Description

I just spent a while trying to figure out why my graph rendering widget wasn't working. It turned out that the object graph I was passing in via options was being deep copied. This meant that I could not rely on ref1 === ref2 on members of the graph. Besides this loss of referential identity I'm also concerned by the potential for:

  • Performance impact on large graphs
  • stack overflows for options containing cyclic objects

At the least, I think you should provide an option to disable this behavior on custom widgets. Workaround is to override _createWidget, but this is ugly.

This was introduced after a discussion here: https://groups.google.com/forum/#!topic/jquery-ui-dev/r8DuMEXYjyU

I think that the downsides were not fully appreciated at the time.

Change History (4)

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

Component: ui.coreui.widget
Owner: set to rattigan
Status: newpending

It's assumed that data sources are arrays, which we copy by reference. We'll need a reduced test case showing problems to even consider a change, but this is highly unlikely to change.

comment:2 Changed 7 years ago by rattigan

Status: pendingnew

My mistake, I was not aware of this behaviour - I'm using an old version prior to this change:

https://github.com/jquery/jquery-ui/commit/b9153258b0f0edbff49496ed16d2aa93bec07d95

I don't see any documentation regarding this behaviour; it might be worth adding, since this is fairly subtle treatment of the options object.

Please feel free to close the ticket!

comment:3 Changed 7 years ago by tj.vantoll

Resolution: notabug
Status: newclosed

It would be nice to have the copy by reference of arrays documented somewhere because that has caused confusion for several users. Not sure where though.

scott.gonzalez: Is $.widget.extend something we would want publicly documented or is that intended for internal use only? Ditto $.widget.bridge.

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

I've filed https://github.com/jquery/api.jqueryui.com/issues/159 to track the documentation issue.

I don't think we want to document $.widget.extend(), but $.widget.bridge() is public. It was created specifically to allow third party developers to use custom constructors. I've filed an issue for that as well: https://github.com/jquery/api.jqueryui.com/issues/160

The bridge was inspired by Fluid's that-ist bridge, was prototyped when I was trying to highlight the pain points of inheritance for jQuery developers, and was subsequently explained by Eric Hynds.

Note: See TracTickets for help on using tickets.