New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle circular reference with toJS #1860
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,11 @@ | ||
import { fromJS, is, List, Map, OrderedMap, Record } from 'immutable'; | ||
import { | ||
fromJS, | ||
is, | ||
List, | ||
Map, | ||
OrderedMap, | ||
Record as ImmutableRecord, | ||
} from 'immutable'; | ||
|
||
import * as jasmineCheck from 'jasmine-check'; | ||
jasmineCheck.install(); | ||
|
@@ -30,7 +37,7 @@ describe('Conversion', () => { | |
list: [1, 2, 3], | ||
}; | ||
|
||
const Point = Record({ x: 0, y: 0 }, 'Point'); | ||
const Point = ImmutableRecord({ x: 0, y: 0 }, 'Point'); | ||
|
||
const immutableData = Map({ | ||
deepList: List.of( | ||
|
@@ -114,6 +121,27 @@ describe('Conversion', () => { | |
); | ||
}); | ||
|
||
it('Throws when calling toJS with a circular reference', () => { | ||
const obj: Record<string, any> = { | ||
foo: 1, | ||
bar: null, | ||
}; | ||
// Create circular reference | ||
obj.bar = obj; | ||
|
||
const simplerTest = List().push(obj); | ||
|
||
expect(() => simplerTest.toJS()).toThrow( | ||
'Cannot convert circular structure to JS' | ||
); | ||
Comment on lines
+134
to
+136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This behavior surprises me, I would have expected a return of |
||
}); | ||
|
||
it('does not throw when using empty objects', () => { | ||
expect(() => fromJS([{}, {}]).toJS()).not.toThrow(); | ||
expect(() => fromJS([['', '']]).toJS()).not.toThrow(); | ||
expect(() => fromJS([[null, null]]).toJS()).not.toThrow(); | ||
}); | ||
|
||
it('Converts deep JSON with custom conversion', () => { | ||
const seq = fromJS(js, function (key, sequence) { | ||
if (key === 'point') { | ||
|
@@ -173,7 +201,7 @@ describe('Conversion', () => { | |
}); | ||
|
||
it('JSON.stringify() respects toJSON methods on values', () => { | ||
const Model = Record({}); | ||
const Model = ImmutableRecord({}); | ||
Model.prototype.toJSON = function () { | ||
return 'model'; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,30 @@ import { Seq } from './Seq'; | |
import { isCollection } from './predicates/isCollection'; | ||
import { isKeyed } from './predicates/isKeyed'; | ||
import isDataStructure from './utils/isDataStructure'; | ||
import isPlainObject from './utils/isPlainObj'; | ||
|
||
export function toJS(value) { | ||
const circularStack = | ||
typeof WeakSet === 'undefined' ? new Set() : new WeakSet(); // WeakSet is not available in IE11. | ||
|
||
return toJSWithCircularCheck(circularStack, value); | ||
} | ||
|
||
function checkCircular(circularStack, value) { | ||
if (!isPlainObject(value)) { | ||
return; | ||
} | ||
|
||
if (circularStack.has(value)) { | ||
throw new TypeError('Cannot convert circular structure to JS'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of throwing here, how about returning |
||
} | ||
|
||
circularStack.add(value); | ||
} | ||
|
||
function toJSWithCircularCheck(circularStack, value) { | ||
checkCircular(circularStack, value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can inline this function since it's only used here |
||
|
||
if (!value || typeof value !== 'object') { | ||
return value; | ||
} | ||
|
@@ -16,13 +38,15 @@ export function toJS(value) { | |
if (isKeyed(value)) { | ||
const result = {}; | ||
value.__iterate((v, k) => { | ||
result[k] = toJS(v); | ||
result[k] = toJSWithCircularCheck(circularStack, v); | ||
}); | ||
return result; | ||
} | ||
const result = []; | ||
value.__iterate(v => { | ||
result.push(toJS(v)); | ||
result.push(toJSWithCircularCheck(circularStack, v)); | ||
}); | ||
circularStack.delete(value); | ||
|
||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)