Bug Tracker

Modify

Ticket #12227 (assigned bug)

Opened 10 months ago

Last modified 4 months ago

$.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:
Blocking: Blocked by:

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.

Change History

comment:1 Changed 10 months ago by jdalton

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

comment:2 Changed 10 months ago by dmethvin

  • Owner set to jdalton
  • Status changed from new to 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?

comment:3 Changed 10 months ago by dmethvin

  • Keywords 1.9-discuss added
  • Status changed from pending to open

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

comment:4 Changed 10 months ago by mikesherov

  • Component changed from unfiled to core

comment:5 Changed 9 months ago by mikesherov

  • Priority changed from undecided to low

comment:6 Changed 8 months ago by mikesherov

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

comment:7 Changed 8 months ago by rwaldron

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

comment:8 Changed 8 months ago by gnarf

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

comment:9 Changed 8 months ago by mikesherov

  • Owner changed from jdalton to demthvin
  • Status changed from open to assigned
  • Milestone changed from None to 1.9

comment:10 Changed 8 months ago by dmethvin

  • Owner changed from demthvin to dmethvin

comment:11 Changed 7 months ago by jdalton

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

comment:12 Changed 7 months ago by dmethvin

@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?

comment:13 Changed 7 months ago by jdalton

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.

comment:14 Changed 4 months ago by jaubourg

#13526 is a duplicate of this ticket.

comment:15 Changed 4 months ago by dmethvin

  • Milestone changed from 1.9 to None

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as assigned
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.