[Fix] stringify: actually fix cyclic references

[liaokunhua]

With all the testing, I think this is the ultimate solution for circular references.

Fixes #403.

[ljharb]

Can you provide a regression test that this fixes?

[liaokunhua]

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 started learning front-end and github a few days ago, starting with a simple project.
I noticed that the stringify.js file in the test report had a line of code that was not covered.

=> throw new RangeError('Cyclic object value');

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.

[ljharb]

Surely you provided some input that caused that error?

[liaokunhua]

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.
It is easy to reproduce.

Based on the original code for testing.
Step1:
modify line 79 in the stringify.js file, throw new RangeError('Cyclic object value'); to throw new Error('Cyclic object value');.

Step2:
Create a new test and select an example of a circular reference in the stringify.js file inside the test folder and make the changes.

t.test("does not crash when parsing circular references", function(st) {
        var circular = { a: 'value' };
        circular.a = circular;
        try {
            qs.stringify(circular);
        } catch(e) {
            console.dir(e)
        }
        st['throws'](
            function () { qs.stringify(circular); },
            RangeError,
            'Cyclic values throw'
        );
        st.end();
})

Step3:
Run test and get output:

TAP version 13
# stringify()
# does not crash when parsing circular references
RangeError: Maximum call stack size exceeded
// ....omitted stack info
ok 1 Cyclic values throw

1..1
# tests 1
# pass  1

# ok

I was dumbfounded!! not the expected "Error:Cyclic object value".

Environment information:
system: ubuntu 20.04
node: v14.16.1
npm: 6.14.12

[ljharb]

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?

[liaokunhua]

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.😅

[ljharb]

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]

Codecov Report

Merging #426 (9aee773) into master (24c19cc) will increase coverage by 0.07%.
The diff coverage is 100.00%.

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

Impacted Files	Coverage Δ
lib/stringify.js	100.00% <100.00%> (+0.70%)	⬆️
test/stringify.js	99.75% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24c19cc...9aee773. Read the comment docs.

[ljharb]

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.