Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Component-class with @observer doesn't dispose reaction if render() is hot-swapped #797

Closed
Venryx opened this issue Nov 12, 2019 · 20 comments · Fixed by #799
Closed

Component-class with @observer doesn't dispose reaction if render() is hot-swapped #797

Venryx opened this issue Nov 12, 2019 · 20 comments · Fixed by #799

Comments

@Venryx
Copy link
Contributor

Venryx commented Nov 12, 2019 •
edited
Loading

[mobx-react version: 6.1.4]

For a React component-class with the @observer decorator, instances of the class do not dispose their MobX reactions, if their render() method is overridden.

For example:

class MyButton extends Component {
	render() {
		return <button>MyButton</button>;
	}
	render_withLogging() {
		console.log("Start");
		return <button>MyButton</button>;
		console.log("End");
	}
	componentDidMount() {
		if (window.enableLogging) {
			this.render = this.render_withLogging;
		}
	}
}

Since the component above hot-swaps the render function, the this.render[mobxAdminProperty] attachment becomes inaccessible to the following code in mobx-react's patched version of componentWillUnmount():

mobx-react/src/observerClass.js

Lines 36 to 40 in a4099e8

patch(target, "componentWillUnmount", function() {
if (isUsingStaticRendering() === true) return
this.render[mobxAdminProperty] && this.render[mobxAdminProperty].dispose()
this[mobxIsUnmounted] = true
})

Become the reaction (stored under the mobxAdminProperty key) is inaccessible, the call to [reaction].dispose() is short-circuited, meaning it never gets cleaned up.

For reference, here is where that special property is first attached: (it only runs the first time the component is rendered, so its hot-swapping of the render function is not guaranteed to be final)

mobx-react/src/observerClass.js

Lines 88 to 89 in a4099e8

reactiveRender[mobxAdminProperty] = reaction
this.render = reactiveRender

Real-world library conflicts

The reason I discovered the issue is because I've been using this library to enable hooks in react class-components: https://github.com/salvoravida/react-class-hooks

It conflicts because, as alluded to above, the library hot-swaps the render function of component instances just like mobx-react does, except it does it even later than mobx-react. (when a hook is first used, rather than when the render function merely begins)

Potential solutions

  1. Attach the reaction (ie. value under mobxAdminProperty) to the class instance rather than the render function.
  2. Patch the this.render to be a getter+setter rather than a property, and code the setter such that, if the value gets replaced, the special mobxAdminProperty property (ie. the reaction) gets transferred from the old render-function to the new one.

I favor the first solution since it doesn't add another level of indirection.

Thoughts?
@Venryx Venryx changed the title Component-class with @observer doesn't dispose reaction if render() is overridden Component-class with @observer doesn't dispose reaction if render() is hot-swapped Nov 12, 2019
@danielkcz
Copy link
Contributor

danielkcz commented Nov 12, 2019 •
edited
Loading

Huh, hot-swapping render? Sounds like a nasty hack :) Never understood this weird desire to have hooks in classes. It's messed up idea imo to have two ways of handling side effects and state.

In my personal opinion, I don't think this should be supported in the library. If you are feeling for it, make a PR with proper tests and let's see what happens next.

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

I was afraid of mentioning the react-universal-hooks library because of this sort of response. XD

This issue was opened because of the potential memory leaks (and slowdown from persistent, unneeded reactions) caused by "late hot-swapping" from any source -- not just react-universal-hooks specifically.

Want an example of a library other than react-universal-hooks that uses the "nasty hack" of hot-swapping render? You're looking at it -- this library. :P

The conflict only happens because both libraries are using hot-swapping, and mobx-react uses fragile hot-swapping -- that is, it assumes it's the only one hot-swapping the function. (That is, it attaches data to its override and assumes it will remain hoisted at this.render)

That's an unwarranted assumption imo, and its conflict with react-universal-hooks is just an example of it.

As mentioned above, there are two potential solutions to it.

  1. No longer make the assumption that mobx-react's hot-swapping is going to be the only one, and therefore attach the reaction to the class instance rather than the render function.
  2. Monkey-patch the this.render to intercept attempts at later hot-swapping -- migrating the attached reaction from the old render-func to the new one.

Either sounds better to me than the current state, where mobx-react silently breaks (ie. has memory leaks and dangling-reactions) if there are any other libraries that apply hot-swapping.

@danielkcz
Copy link
Contributor

Yea, as I said, feel free to make a PR with a solution and if it turns out not invasive too much without breaking too many things, we might consider merging it.

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

Okay, sounds good.

I suspect it is as simple as changing the two lines that access this.render[mobxAdminProperty] to instead access this[mobxAdminProperty], but we'll see.

@mweststrate
Copy link
Member

mweststrate commented Nov 12, 2019 via email

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

Just do the swapping / if check inside the render?

I'm not sure what you mean.

Or copy all own members of the original function onto your new render function?

If the only source of this.render swapping were the project developer, this could work. However, some libraries rely on modifying/wrapping this.render. For example, mobx-react, react-universal-hooks, and some custom (in npm-module) decorators I use.

At the very least, I think mobx-react should warn if another library tries to swap this.render, as currently it just silently creates a memory leak (and leaves a dangling reaction that isn't actually needed anymore).

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

For reference, I've created this test to make clear what's going wrong: (the test is currently failing)

test("#797 - replacing this.render should not break disposing of reaction", () => {
    @observer
    class Component extends React.Component {
        render() {
            return <div/>
        }
        swapRenderFunc() {
            this.render = () => {
                return <span/>
            }
        }
    }

    const compRef = React.createRef()
    const { unmount } = render(<Component ref={compRef} />)

    const reaction = compRef.current.render[$mobx]
    expect(reaction.isDisposed).toEqual(false)

    compRef.current.swapRenderFunc()
    unmount()
    expect(reaction.isDisposed).toEqual(true)   << test is failing here
})

@Venryx Venryx mentioned this issue Nov 12, 2019
Closed
@mweststrate
Copy link
Member

Or copy all own members of the original function onto your new render function?

That would probably look like:

	componentDidMount() {
		if (window.enableLogging) {
                         const source = this.render
			this.render = this.render_withLogging;
                         for (let key in source) this.render[key] = this.source[key]
		}
	}

(might need a different way of enumeration depending on the enumerability of the original function)

By the way, I doubt that doing the above actually will enable logging reliable, as mobx-react IIRC captures the original render in it's reaction closure, so a swapped render will probably totally be ignored when a re-render is caused by an observable change.

(That all being said, I personally don't get why one would want hooks in a component class (I mean, wouldn't just refactoring the then not be a lot more intuitive). But I even less get that why do that for an observer component, as you can leverage all mobx primitives for managing side effects already)

@mweststrate
Copy link
Member

Just as a head up: We probably won't accept a PR unless it is a really trivial change on our side, for the simple reason that the above library isn't significant enough yet to warrant the significant additional complexity on maintenance of this one while there are clear work arounds (such as splitting the component in 2, using <Observer> instead, etc.

Btw, not patching render methods is a documented limitation of mobx-react (the simple case of render = () => { } is already forbidden in observer components).

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

That would probably look like: [...]

Yeah, I think that would work fine for a project developer's own swapping of this.render, but it's not really a solution for cases where third-party libraries perform the swapping.

(That all being said, I personally don't get why one would want hooks in a component class (I mean, wouldn't just refactoring the then not be a lot more intuitive). But I even less get that why do that for an observer component, as you can leverage all mobx primitives for managing side effects already)

I agree that the usefulness of hooks in component classes goes down a lot when integrating MobX. My project used to use Redux, so I wasn't able to use external state as liberally (since connect functions have a higher overhead than MobX dependency trees), leading to me using local class variables for some complex behavior.

One place where hooks are still useful in component classes is for the "useCallback" hook -- enabling you to only have a callback updated when a variable it uses is changed. This improves performance by letting the children components use React.PureComponent to not re-render if no props have changed.

Another place it's useful is for using third-party libraries that provide functionality nicely wrapped in "useEffect" (thus requiring hooks). For example: https://github.com/rehooks/component-size

But anyway, that's just the library that made me detect this issue; even if react-universal-hooks did not exist, I'd still think it's worth fixing this issue for other libraries that might try to modify this.render.

We probably won't accept a PR unless it is a really trivial change on our side

Well, I created this PR for it just now: #798

The modification used to make the fix seems pretty straight-forward/trivial to me; it's just attaching the reaction to the class instance instead of the render function.

@mweststrate
Copy link
Member

mweststrate commented Nov 12, 2019 via email

@mweststrate
Copy link
Member

mweststrate commented Nov 12, 2019 via email

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

Good point. Just tested, and component renders triggered by MobX do not call into a newly-swapped-in this.render function.

For example this fails:

test("#797 - replacing this.render should not break disposing of reaction", () => {
    let newRenderFuncCalled = false
    @observer
    class Component extends React.Component {
        @observable
        thing = 1

        render() {
            return <div />
        }
        swapRenderFunc() {
            this.render = () => {
                newRenderFuncCalled = true
                return <span>{this.thing}</span>
            }
        }
    }

    const compRef = React.createRef()
    const { unmount } = render(<Component ref={compRef} />)

    const reaction = compRef.current[mobxAdminKeyForRender]
    expect(reaction.isDisposed).toEqual(false)

    compRef.current.swapRenderFunc()
    //compRef.current.forceUpdate()         // new render-func runs if triggered by this
    compRef.current.thing = 5               // but not if by this
    expect(newRenderFuncCalled).toEqual(true)   << fails here, if triggered by MobX change

    unmount()
    expect(reaction.isDisposed).toEqual(true)
})

I suppose that a proper fix would therefore require a more substantial reworking of mobx-react's render-func wrapping. (while my PR fixes the particular bug in MobX, it doesn't solve the wider problem of MobX not having a reference to the new this.render in order to have it actually called)

I don't have time/motivation for that "proper fix" right now, so I'll close the pull-request since it's incomplete.

Might be worth keeping this issue open for now though, since mobx-react should probably at least log a warning if something tries to modify the render function after mobx accesses and wraps it, as it can lead to silent memory leaks as mentioned earlier.

Adding something like this to mobx-react's patched componentWillUnmount might work:

if (this.render[$mobx] == null) {
    console.warn(
        "The render function for an @observer component (${this.constructor.name}) was modified after MobX attached. " +
        "This is not supported, since the new function can't be triggered by MobX."
    );
}

@danielkcz
Copy link
Contributor

Feel free to make a PR with a warning, otherwise, I don't think we are too keen supporting something like that. Perhaps it will help other people to realize that trying to mix hooks and classes is a bad idea :)

@mweststrate
Copy link
Member

mweststrate commented Nov 12, 2019 via email

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

Okay, a warning would be good enough for me. (assuming the "proper fix" would require a substantial mobx-react rework)

I mainly just didn't want others to hit the "hidden bug" of lots of MobX reactions unknowingly leaking / persisting past component lifecycle.

(For my particular usage of react-universal-hooks, I'll try working with the author to create a workaround to make it compatible with mobx-react -- for example, having its replacement of this.render occur before mobx-react's replacement.)

Venryx added a commit to Venryx/mobx-react that referenced this issue Nov 12, 2019
@Venryx
* Added warning for attempting to modify a component class' render fu…
edb5779 
…nction after a MobX reaction has already attached. Helps prevent memory leaks as in: [mobxjs#797](mobxjs#797)

* Added test for the above.
* Added changelog entry.
@Venryx Venryx mentioned this issue Nov 12, 2019
Merged
@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

Created a pull-request (#799) that adds a warning for the issue (replacing this.render after MobX has attached).

If one needs to do so anyway, here is a complete example of how you can, while preventing the leaking/persistence of the MobX reaction for the component:

@observer
class Test1 extends Component<{}, {}> {
	render() {
		return <div/>;
	}
	componentDidMount() {
		setTimeout(()=>this.swapRenderFunction(), 1000); // simulate delay
	}
	swapRenderFunction() {
		const oldRender = this.render;
		this.render = function () {
			// <<< add wrapper code here
			return oldRender.apply(this, arguments);
		};

		// copy over mobx administration object
		for (const symbol of Object.getOwnPropertySymbols(oldRender)) {
			this.render[symbol] = oldRender[symbol];
		}
	}
}

However, some drawbacks:

  1. Make sure you don't add "reactive" code to the new wrapper, as the things it accesses won't be added to the MobX dependency-tree.
  2. The wrapper code won't be called if MobX is the one that triggers the component's re-render! (thus it has pretty limited usefulness)

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

A better way than the above (because of issue 2 mainly), is actually just to make sure the wrapper you want to place around this.render, is applied prior to the first time this.render is called (since that is when MobX attaches its own wrapper).

Actually, that won't work currently because the class-decorator stores a reference to the class prototype's render function even prior to the first call to render. This workaround would be enabled if mobx-react changed this:

const baseRender = target.render
target.render = function() {
    return makeComponentReactive.call(this, baseRender)
}

To this:

target.render = function() {
    return makeComponentReactive.call(this, this.render)
}

Is the fix above straight-forward enough to be accepted as a PR? It would enable compatibility with other libraries (or custom code) that modify this.render (prior to first render), without causing any architectural changes in mobx-react.

EDIT: Nevermind, the above won't work, because it will cause an infinite loop -- when the 3rd-party lib's wrapper-func is created, it will reference the mobx original on-class wrapper-func, meaning that if mobx's final on-instance this.render wrapper-func calls the 3rd-party lib's wrapper-func, it will call the mobx on-class wrapper-func, which would then try to re-attach a mobx on-instance wrapper-func!

Technically one could probably make mobx-react compatible with this case, by having the on-class wrapper-func recognize it and just call the original on-class render-func instead of attaching a new on-instance wrapper-func. But this is confusing enough that I fully understand the mobx-react devs not wanting to add it.

Like always, if one absolutely needs a given change in library behavior, you can always either fork the library (hard / high maintenance), or use this (easy -- though requires care to not make mismatches between the string-replace code and the lib version marked in package.json).

@Venryx
Copy link
Contributor Author

Venryx commented Nov 12, 2019 •
edited
Loading

Since I don't see a clear path forward to making mobx-react compatible with hot-swapping of this.render, I will close this issue.

If someone thinks of a good solution (that isn't confusing like the last option listed above), feel free to bring it up.

As of now, I'll find some other way to integrate the libraries that use this.render hot-swapping. (perhaps by making pull-requests for them that use alternate ways of implementing their behavior)

@Venryx Venryx closed this as completed Nov 12, 2019
@Venryx Venryx mentioned this issue Nov 12, 2019
Open
@lock
Copy link

lock bot commented Jan 14, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@danielkcz @mweststrate @Venryx