#12227 closed bug (wontfix)

Opened August 09, 2012 06:53AM UTC

Closed June 29, 2013 07:49PM UTC

Last modified February 19, 2014 08:55PM UTC

$.extend(true, ....) won't work on sources that contain circular references.

Reported by:	jdalton	Owned by:	dmethvin
Priority:	low	Milestone:	None
Component:	core	Version:	1.8rc1
Keywords:	1.9-discuss	Cc:
Blocked by:		Blocking:

Description

Try this page and open your web inspector console:

http://jsbin.com/ehisub

      var object = {
        'foo': { 'a': 1 },
        'bar': { 'a': 2 }
      };

      var source = {
        'foo': { 'b': { 'foo': { 'c': { } } } },
        'bar': { }
      };

      source.foo.b.foo.c = source;
      source.bar.b = source.foo.b;

      var actual = $.extend(true, object, source); // stack overflow

I handle this in Lo-Dash here:

https://github.com/bestiejs/lodash/blob/b5b8523257b46aafa180e9d6e256e6db2f3a5d2c/lodash.js#L2034-2045

using the stack array to hold objects I've iterated over and then return the corresponding assigned value.

Attachments (0)

Change History (17)

Changed August 10, 2012 04:50PM UTC by jdalton comment:1

The current $.extend has a basic check to attempt to avoid stack overflows but it's not enough.

Changed August 21, 2012 01:25AM UTC by dmethvin comment:2

owner:	→ jdalton
status:	new → pending

I'm not sure this is something we need to guard against. Keeping a stack of visited objects seems like overkill. It feels like our internal methods are being forced to handle cases we don't encounter in our own usage. When might we encounter something like this?

Changed August 27, 2012 05:15PM UTC by dmethvin comment:3

keywords:	→ 1.9-discuss
status:	pending → open

Our "internal" utility methods seem to be pulled into dealing with all sorts of edge cases lately. Thoughts?

Changed September 05, 2012 03:20AM UTC by mikesherov comment:4

component:	unfiled → core

Changed September 10, 2012 01:02PM UTC by mikesherov comment:5

priority:	undecided → low

Changed October 14, 2012 10:37PM UTC by mikesherov comment:6

-1, That's what Lo-Dash is for.

Changed October 14, 2012 11:38PM UTC by rwaldron comment:7

-1, I really appreciate the report, but I'd rather point users to Lo-Dash (as Mike noted)

Changed October 22, 2012 05:43PM UTC by gnarf comment:8

+0, If the impact to code size is small enough, this seems like one of those edge cases people might run into.

Changed October 29, 2012 05:25PM UTC by mikesherov comment:9

milestone:	None → 1.9
owner:	jdalton → demthvin
status:	open → assigned

Changed October 29, 2012 05:26PM UTC by dmethvin comment:10

owner:	demthvin → dmethvin

Changed November 19, 2012 07:58PM UTC by jdalton comment:11

Just to clarify deep $.extend is not an internal method. It's used by devs wanting a deep clone equiv.

The bug report was to fix an existing chunk of code in jQuery which seems to be in place to handle this issue (circular references), but does not. So this isn't about adding new support, it's about fixing the existing support.

Keeping track of visited objects is nothing new or extreme. Underscore does this in its _.isEqual method as well.

Lo-Dash, with its extra checks and circular reference support is still faster than jQuery (I'm guessing because Lo_Dash punts on sparse arrays).

http://jsperf.com/deep-extend-perf#chart=bar

Changed November 19, 2012 08:07PM UTC by dmethvin comment:12

@jdalton, the reason I quoted "internal" was because yes we've documented this method and people are using it as a generalized any-kind-of-object extension mechanism where we even try to determine array-likeness to decide how to extend it. The method was really not originally designed to carry that load, it started as a simple method to merge simple plain objects. So it's just me expressing frustration that these methods get pulled into progressively more complex scenarios.

Would you be interested in filing a pull request for this?

Changed November 19, 2012 08:26PM UTC by jdalton comment:13

Would you be interested in filing a pull request for this?

Yap, can do, I'll try not to let this fall in the cracks.

Changed February 27, 2013 02:26PM UTC by jaubourg comment:14

#13526 is a duplicate of this ticket.

Changed March 01, 2013 05:48PM UTC by dmethvin comment:15

milestone:	1.9 → None

Changed June 29, 2013 07:49PM UTC by dmethvin comment:16

resolution:	→ wontfix
status:	assigned → closed

I'm inclined to let this be the domain of lodash, since nobody has demonstrated a case where our internal use needs to protect against it. I created an api docs ticket.

https://github.com/jquery/api.jquery.com/issues/332

Changed February 19, 2014 08:55PM UTC by dmethvin comment:17

#14821 is a duplicate of this ticket.

Side navigation