[Fix] stringify: actually fix cyclic references#426
Conversation
Fixes ljharb#403. Co-authored-by: liaokunhua <acutedev@163.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>
ljharb
left a comment
There was a problem hiding this comment.
Can you provide a regression test that this fixes?
Sorry bro, this is a bit out of my league, I haven't thought of a special object that would prove i didn't fix this bug. So, let's talk about how I found this bug. => I did some tests, the result is that non-circular duplicated references can work, but circular dependencies result in "RangeError: Maximum call stack size exceeded", not "RangeError: Cyclic object value". I think the exception thrown in the use case test of the circular dependency happens to be RangeError, but the exception message is not checked. |
|
Surely you provided some input that caused that error? |
Right, an example of a circular reference is constructed, which is inside the use case test. Based on the original code for testing. Step2: Step3: I was dumbfounded!! not the expected "Error:Cyclic object value". Environment information: |
|
ah, i see - so changing that test to check the error message - so we know it's not a stack overflow - would reproduce the error that this PR fixes? if so, could you make that change? |
emmm...kinda wish it was me, but i don't think i have it in me yet.😅 |
stringify: fix cyclic referencestringify: actually fix cyclic references
|
I rebased this PR, and updated the test cases in the default branch that were asserting a RangeError, but not the right one, and added another test case that confirms a fix for #403. |
Codecov Report
@@ Coverage Diff @@
## master #426 +/- ##
==========================================
+ Coverage 99.78% 99.85% +0.07%
==========================================
Files 8 8
Lines 1393 1408 +15
Branches 169 172 +3
==========================================
+ Hits 1390 1406 +16
+ Misses 3 2 -1
Continue to review full report at Codecov.
|
ljharb
left a comment
There was a problem hiding this comment.
actually i'll merge this as-is, to fix the bug; but i'd love it if there was a followup to clean up the loop.
With all the testing, I think this is the ultimate solution for circular references.
Fixes #403.