#12227 closed bug (wontfix)
$.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.
Change History (17)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Owner: | set to 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?
comment:3 Changed 10 years ago by
Keywords: | 1.9-discuss added |
---|---|
Status: | pending → open |
Our "internal" utility methods seem to be pulled into dealing with all sorts of edge cases lately. Thoughts?
comment:4 Changed 10 years ago by
Component: | unfiled → core |
---|
comment:5 Changed 10 years ago by
Priority: | undecided → low |
---|
comment:7 Changed 10 years ago by
-1, I really appreciate the report, but I'd rather point users to Lo-Dash (as Mike noted)
comment:8 Changed 10 years ago by
+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 10 years ago by
Milestone: | None → 1.9 |
---|---|
Owner: | changed from jdalton to demthvin |
Status: | open → assigned |
comment:10 Changed 10 years ago by
Owner: | changed from demthvin to dmethvin |
---|
comment:11 Changed 10 years ago by
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 10 years ago by
@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 10 years ago by
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:15 Changed 10 years ago by
Milestone: | 1.9 → None |
---|
comment:16 Changed 10 years ago by
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.
The current
$.extend
has a basic check to attempt to avoid stack overflows but it's not enough.