Handle circular reference with toJS #1860
Conversation
| List, | ||
| Map, | ||
| OrderedMap, | ||
| Record as ImmutableRecord, |
There was a problem hiding this comment.
Instead of aliasing the Record name here, can you just make an explicit type in the unit test? (Does that value even need to be explicitly typed?)
| expect(() => simplerTest.toJS()).toThrow( | ||
| 'Cannot convert circular structure to JS' | ||
| ); |
There was a problem hiding this comment.
This behavior surprises me, I would have expected a return of [obj]
| } | ||
|
|
||
| if (circularStack.has(value)) { | ||
| throw new TypeError('Cannot convert circular structure to JS'); |
There was a problem hiding this comment.
Instead of throwing here, how about returning circularStack.get(value) to recreate the cycle?
| } | ||
|
|
||
| function toJSWithCircularCheck(circularStack, value) { | ||
| checkCircular(circularStack, value); |
There was a problem hiding this comment.
You can inline this function since it's only used here
|
IMHO the original mistake was the My advice here is to attempt to resolve the cycle instead of simply throwing, since it's probably not uncommon to find this kind of cycle in regular JS objects stored within a collection, but perhaps we should consider a future breaking change which simply stops the recursive conversion when it finds a non-Immutable.js collection/value |
Fixes #1789