Using Map method with React causes infinite recursion

[scriptguru]

I am not 100% sure what's happening as my research was limited. Still I'll try to explain the problem.

var constants = immutable({
        BASE_PATH: '/my/files'
    }),
    BASE = constants.BASE_PATH,
    menu =  [
        {
            link: BASE,
            title: 'Files'
        },
        {
            link: `${BASE}/:sharing`,
            title: 'Sharing'
        }
    ];

...

    render() {

        var moreParams = ( this.props.params.splat || '' ).split('/'),
            command = moreParams.length && moreParams[0].charAt(0) == ':' ? moreParams[0].substr(1) : null;

        return (
            <div className="_personal-storage">
                <Menu items={ menu } />
                <Storage base={ BASE } callback={ processCommand }>
                    <h1>Personal storage</h1>
                </Storage>
            </div>
        );
    }

Everything works this way, but as soon as I make menu immutable, it goes to the weirdest infinite recursion in seamless-immutable.js:

Uncaught RangeError: Maximum call stack size exceeded
defineProperty  @   (program):779
addPropertyTo   @   seamless-immutable.js:5
makeImmutableObject @   seamless-immutable.js:342
Immutable   @   seamless-immutable.js:371
Immutable   @   seamless-immutable.js:367
Immutable   @   seamless-immutable.js:367
Immutable   @   seamless-immutable.js:367
makeImmutableArray  @   seamless-immutable.js:106
Immutable   @   seamless-immutable.js:354
Immutable   @   seamless-immutable.js:367

The weirdest part is - it happens only if I pass menu as a property to a react component. If I just log it to the console it works just fine. It doesn't matter if I make it immutable on declaration or later - it works the same.

When I've been debugging it, I couldn't believe my eyes - after making immutable my objects, it started making immutable React stuff {$$typeof: Symbol(react.element), key: null, ref: null, props: Object, _owner: ReactCompositeComponentWrapper…}$$typeof: Symbol(react.element)_owner: ReactCompositeComponentWrapper_self: null_source: null_store: Objectkey: nullprops: Objectref: nulltype: _default(props)__proto__: Object and this is where the infinite recursion happens. But why does it make it immutable at all? I have no idea.

[sudhakar]

What does your Menu component look like? Might be related to #59

[scriptguru]

The thing is, it doesn't do anything fancy, and this bug appears with any component

    render() {

        return (
            <ul className="generalMenu">
                {
                    ( this.props.items || [] ).map( ( item ) => <li key={ item.cssClass } className={ item.cssClass }><Link to={ item.link }>{ item.title }</Link></li> )
                }
            </ul>
        );
    }

[sudhakar]

Ah.. this.props.items is immutable menu. When you call map, the return value(which is array of react elements) is also casted into immutable see https://github.com/rtfeldman/seamless-immutable/blob/master/src/seamless-immutable.js#L97 same as reported in #59

I havent personally tested it though but it looks like Immutable()-izing React elements causes overflows

[scriptguru]

Hm, this is an interesting point. I see what's the problem now, thanks for explanation.

To me it looks like some checks have to be added to seamless-immutable to avoid this issue.

I understand that seamless-immutable hasn't been created for React, but let's face it - it's becoming popular largely thanks to React, so it shouldn't cause extreme problems when being used with it.

React promotes ImmutableJS, which is obviously not as convenient as seamless-immutable, but at least it doesn't cause infinite recursion or something like that.

As a workaround, I think for now I can just use lodash map method, which IIRC doesn't use the native method, but definitely this issue should be addressed since in most examples regarding React usage native methods like map are being used.

UPDATE: as expected, it works fine when using lodash map method.
As I understand, React uses circular references and that's how infinite recursion happens. Probably that's what should be detected, not the React itself (however, detecting React could be an easy first step for solving the problem if the good solution is not obvious).

[sudhakar]

IMHO infinite recursion is certainly a bug. Atleast better error should be thrown when circular references are found. Probably you may want to update issue title to get better visibility

[scriptguru]

Good idea, thanks

[sudhakar]

Oops..Just title change is good enough..It helps to identify in issues list. On first glance, your issue wasn't entirely clear.

BTW, its not a major problem per se. Just that it fails to handle a special case with react. I would simply rephrase it as "Using Map method with react causes infinite loop". (oh pls do remove all bold text in description. Its too noisy)

Also it is not a simple fix. Where references needs to be deeply compared or whatever. Other solution would be to publish methods which will return Mutable data. Again these are major changes that needs to be carefully considered & it might even be against the target goals of the library.

Sorry my last comment was little vague. So lets be nice here ;)

[scriptguru]

Yes, I know it's not an easy fix. One of the possible solutions would be to use WeakMap or Map, using all objects that are being converted to immutable as keys, but unfortunately it won't work in older browsers, so it's not really an option. That's why I suggested to include a quick fix that would resolve it just for usage with React, just because it's a major use case.

Or at least a note could be added to the Readme.md on this particular issue, helping people to avoid this problem.

[jedwards1211]

I wouldn't recommend using WeakMap or Map even with polyfills...the polyfills aren't perfect and can cause memory leaks in some situations, especially if the same object is used as a key in many different Maps/WeakMaps.

[scriptguru]

I think this problem can be solved with one of following approaches or their combination:

1. Recursion counter. If we go let's say 50 levels deep, it's a bad sign. The counter is basically free for CPU, but cannot really detect circular references - just how deep we are.
2. Using JSON.stringify() on an object with circular references will throw an error, and this can be used to detect them. However, it will also throw an error in some other cases too - when in encounters something that cannot be converted to JSON. And I guess it's way more expensive than counter.

I think, counter with some reasonable limit, and with possibility to adjust the limit would work the best. Currently a property that is being used as an immutability marker has value true. However, it could store the maximum allowed recursion level, that user can optionally set when making an object immutable.

[jedwards1211]

@scriptguru when you use objects as keys with the ES2015 Map polyfill, it just stores the values inside a hidden property tree on the key objects anyway. That's the only way it's possible to simulate object keys in ES5. I think the sole reason old browsers aren't supported is that they don't have an Object.defineProperty method, so the Map data can't be created in a hidden/non-enumerable property.

But that's no matter for this method. All you have to do is temporarily tack the output immutable onto each input object/array in a property with a name that no one's going to use, like _$_$_$immutable, and then remove those properties when done converting the tree into an immutable. I'd be a lot more comfortable with this than with a recursion counter or JSON.stringify() -- the graph structure of the input should be converted to an immutable with the same graph structure, and using a Map or keys on the input objects is the only way to do that.