Maximum call stack size exceeded error in lifecycle methods

[smikula]

I encountered this error when upgrading to v5.3.2. (It doesn't happen in 5.2.x.) The issue is that certain lifecycle methods end up recursing infinitely—the callstack ends up looking something like this:

RangeError: Maximum call stack size exceeded
    at c.wrapperMethod
    at c.f.componentDidMount
    at c.wrapperMethod
    at c.f.componentDidMount
    at c.wrapperMethod
    at c.f.componentDidMount
    at c.wrapperMethod
    at c.f.componentDidMount
    at c.wrapperMethod
    at c.f.componentDidMount
    ....

It appears this happens when @observer patches a component's lifecycle methods and something else is patching those methods as well, such as another decorator or a base class. (For instance the BaseComponent in office-ui-fabric-react does this.) I haven't dug into the mobx-react code enough to grok exactly what the problem is, but I've seen multiple instances of mobx-react interacting badly with other libraries that do this sort of thing.

[mweststrate]

@smikula does this problem occur when using disposeOnUnmount, or does it happen in any case? Also could you give the definition of the offending component? (function bodies can be left out if it is sensitive or big)

Background: we changed the method patching mechanism in 5.3 to help people that accidentally write stuff like componentWillMount = () => { } instead of componentWillMount() { }, (in the first case the patched prototype methods would be overwritten). However, if this introduces new problems, we could probably revert

cc @xaviergonz

[xaviergonz]

I'll take a look this afternoon and try to fix it, it seems that class also does its own patching of methods its own way

[mweststrate]

Yeah current solution might be too complicated, maybe we should go back to the old monkey-patching mechanism, as that will play nice with other libs, and have the disposeOnunmount create a componentWillUnmount only on the own instance, calling the prototype in turn. (That would preserve the correct behavior if the prototype is patched later on)

[xaviergonz]

but I think it won't play nice with the patching from observer?

[xaviergonz]

I think I found the problem, will create a PR later

[xaviergonz]

PR ready

[mweststrate]

Should be fixed in 5.3.4

[smikula]

@mweststrate @xaviergonz

I tried 5.3.4 and it worked. However, I just tried out 5.3.5 and am hitting what appears to be this issue again, with a slightly different callstack:

RangeError: Maximum call stack size exceeded
    at Array.concat
    at c.fn
    at c.f.componentDidMount
    at c.wrapper
    at c.fn
    at c.f.componentDidMount
    at c.wrapper
    at c.fn
    at c.f.componentDidMount
    at c.wrapper
    ...

[xaviergonz]

that's weird, there's now a unit test which uses the BaseComponent in office-ui-fabric-react to make sure it doesn't happen. Could you share a component where it happens?

[mweststrate]

I suggest we go back to classic patching, which is less obtrusive and where the edge cases are easier to grasp? Op wo 24 okt. 2018 06:51 schreef Javier Gonzalez <notifications@github.com>:

…

that's weird, there's now a unit test which uses the BaseComponent in office-ui-fabric-react to make sure it doesn't happen. Could you share a component where it happens? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#579 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhC_Td0wjUUvWTz1HcPGN2RB7ojpyks5un_G2gaJpZM4XvYcz> .

[xaviergonz]

I'd like to give it one last try, because to use the old patching method with disposeOnUnmount would make disposeOnUnmount depend on the class being marked with observer to work (not to mention that I think the old patching mechanism invoked the mixins multiple times when it was using inheritance and both the base and the inherited class were marked as observer, but I'm not 100% sure)

(also I'm extremely curious what is the missing case given that the unit tests now cover that very same base component as well as recursive calls)

in the worst case recursive calls could be totally prevented like in the previous fix, but I'd rather avoid that

@smikula could you provide a simple component that exhibits that behavior?

[xaviergonz]

that being said, I so wished react had global lifecycle hooks / events

[smikula]

Okay, I dug into it a bit on my side. First off, the reason your locking logic doesn't catch it is that the recursion happens via realMethod.apply(this, args) which is outside the lock.

Second, it turns out this is due to interacting with different code, in this case another decorator that also overrides componentDidMount. This code is internal so I can't point you to it, but here's the gist of the decorator:

function logMountingPerformance() {
    return (target: any) => {
        var original = target;
        var instance;

        // a utility function to generate instances of a class
        function construct(oldConstructor: Function, args: any) {
            var c: any = function(this: any) {
                return oldConstructor.apply(this, args);
            };
            c.prototype = oldConstructor.prototype;
            instance = new c();
            logComponentConstructionTime();
            return instance;
        }

        // the new constructor behaviour
        var f: any = function(...args: any[]) {
            return construct(original, args);
        };

        var originalComponentDidMount = original.prototype.componentDidMount;

        // copy prototype so intanceof operator still works
        f.prototype = original.prototype;

        f.prototype.componentDidMount = function() {
            var returnValue;
            if (originalComponentDidMount) {
                returnValue = originalComponentDidMount.apply(instance);
            }
            logComponentMountingTime();
            return returnValue;
        };

        // return new constructor (will override original)
        return f;
    };
}

[xaviergonz]

so you use it like

@logMountingPerformance()
@observer
class C extends React.Component {...}

I guess?

If you write it like

@observer 
@logMountingPerformance()
class ...

it works, but nevertheless I'll take a look, thanks!

[xaviergonz]

@mweststrate Got it, PR created

[mweststrate]

Released as 5.3.6

[mweststrate]

that being said, I so wished react had global lifecycle hooks / events

@xaviergonz check #588 / https://reactjs.org/docs/hooks-intro.html ;-)

[xaviergonz]

@mweststrate nice, didn't know about those :D
but not quite what I had in mind, I was thinking more of a way like

const disposer = registerComponentLifecycleHook((componentInstance, lifecycle) => {
  if (lifecycle === "componentWillUnmount" && componentInstance[mobxMixins]) {
    // run all disposeUnmount mixins
  }
})

basically that'd avoid all the silly patching

of course one could argue that if instead of using a decorator we were using the <Observer> component as a high order component none of this would be needed as well (basically I mean that observer itself created a component where on render it would render the inner component, rather than patch it

in other words

class C {...}
const OC = observer(C) 
// result
<Observer><C {...this.props}/></Observer>
component that will render C with the same props passed to observer
both Observer and C will have their own componentDidMount, shouldComponentUpdate, etc

[xaviergonz]

hmm actually, why doesn't (@)observer work like this?
if it is for legacy reasons there could be a new decorator/function or a new version