Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 31 additions & 3 deletions __tests__/Conversion.ts
@@ -1,4 +1,11 @@
import { fromJS, is, List, Map, OrderedMap, Record } from 'immutable';
import {
fromJS,
is,
List,
Map,
OrderedMap,
Record as ImmutableRecord,
Copy link
Collaborator

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?)

} from 'immutable';

import * as jasmineCheck from 'jasmine-check';
jasmineCheck.install();
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior surprises me, I would have expected a return of [obj]

});

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') {
Expand Down Expand Up @@ -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';
};
Expand Down
28 changes: 26 additions & 2 deletions src/toJS.js
Expand Up @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of throwing here, how about returning circularStack.get(value) to recreate the cycle?

}

circularStack.add(value);
}

function toJSWithCircularCheck(circularStack, value) {
checkCircular(circularStack, value);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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;
}
Expand All @@ -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;
}