Skip to content

Commit

Permalink
* Added warning for attempting to modify a component class' render fu…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
Venryx committed Nov 12, 2019
1 parent 78c6b49 commit edb5779
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 7 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# MobX-React Changelog

### 6.1.5

- Added warning for attempting to modify a component class' render function after a MobX reaction has already attached. Helps prevent memory leaks as in: [#797](https://github.com/mobxjs/mobx-react/issues/797)

### 6.1.4

- Update dependency [email protected] which includes fix for [RN Fast Refresh](https://github.com/mobxjs/mobx-react-lite/issues/226)
Expand Down
26 changes: 19 additions & 7 deletions src/observerClass.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,29 @@ export function makeClassComponentObserver(componentClass) {
}
patch(target, "componentWillUnmount", function() {
if (isUsingStaticRendering() === true) return
this.render[mobxAdminProperty] && this.render[mobxAdminProperty].dispose()
if (this.render[mobxAdminProperty]) {
this.render[mobxAdminProperty].dispose()
} else {
const displayName = getDisplayName(this)
console.warn(
`The render function for an observer component (${displayName}) was modified after MobX attached. This is not supported, since the new function can't be triggered by MobX.`
)
}
this[mobxIsUnmounted] = true
})
return componentClass
}

// Generates a friendly name for debugging
function getDisplayName(comp) {
return (
comp.displayName ||
comp.name ||
(comp.constructor && (comp.constructor.displayName || comp.constructor.name)) ||
"<component>"
)
}

function makeComponentReactive(render) {
if (isUsingStaticRendering() === true) return render.call(this)

Expand All @@ -55,12 +72,7 @@ function makeComponentReactive(render) {
*/
setHiddenProp(this, isForcingUpdateKey, false)

// Generate friendly name for debugging
const initialName =
this.displayName ||
this.name ||
(this.constructor && (this.constructor.displayName || this.constructor.name)) ||
"<component>"
const initialName = getDisplayName(this)
const baseRender = render.bind(this)

let isRenderingPending = false
Expand Down
30 changes: 30 additions & 0 deletions test/observer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -838,3 +838,33 @@ test.skip("#709 - applying observer on React.memo component", () => {
// eslint-disable-next-line no-undef
render(<Observed />, { wrapper: ErrorCatcher })
})

test("#797 - replacing this.render should trigger a warning", () => {
@observer
class Component extends React.Component {
render() {
return <div />
}
swapRenderFunc() {
this.render = () => {
return <span />
}
}
}

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

const msg = []
const warn_orig = console.warn
console.warn = m => msg.push(m)

unmount()

expect(msg.length).toBe(1)
expect(msg[0]).toBe(
`The render function for an observer component (Component) was modified after MobX attached. This is not supported, since the new function can't be triggered by MobX.`
)
console.warn = warn_orig
})

0 comments on commit edb5779

Please sign in to comment.