Fix memory leak when using server-side #11

the83 · 2018-06-01T20:54:26Z

I recently ran into a memory leak when using this server-side. This occurs because the visibility handlers are set up in the constructor instead of a lifecycle hook, and because componentWillUnmount does not execute server-side, we never stop observing the element. I set up a simple example of this happening here: https://github.com/the83/memory-leak-example

This moves the setup into the componentDidMount hook so that we don't even observe visibility at all server-side.

tazsingh

Good catch @the83! Thanks for the PR and apologies for the delayed response - I was on vacation for the past few weeks.

I've requested a few changes here. Let me know if you have any questions. But I'm happy to merge this and cut a release as soon as it's ready 👍

tazsingh · 2018-06-17T18:40:33Z

src/createLoadableVisibilityComponent.js

@@ -1,8 +1,8 @@
-import React, { Component } from 'react'
+import * as React from 'react'


Would you be able to revert this change? I'd prefer to be explicit about imports.

tazsingh · 2018-06-17T18:42:11Z

src/__tests__/loadable-components/intersection-observer.test.js

@@ -7,6 +7,7 @@ const { IntersectionObserver, makeElementsVisible } = require('../../__mocks__/I
 global.IntersectionObserver = IntersectionObserver

 const loadableVisiblity = require('../../loadable-components')
+const trackedElements = require('../../tracked_elements').default


It looks like this is primarily being used for the tests. Would you be able to modify the IntersectionObserver mock to include this behaviour instead?

tazsingh · 2018-06-17T18:43:37Z

src/createLoadableVisibilityComponent.js

@@ -95,7 +77,7 @@ function createLoadableVisibilityComponent (args, {
        return <div
          style={{display: 'inline-block', minHeight: '1px', minWidth: '1px'}}
          className={this.props.className}
-          ref={this.attachRef}
+          ref="loading"


Can you revert this change? String refs have been deprecated - https://reactjs.org/docs/refs-and-the-dom.html#legacy-api-string-refs

tazsingh · 2018-06-17T18:53:50Z

src/createLoadableVisibilityComponent.js

@@ -22,63 +22,45 @@ function createLoadableVisibilityComponent (args, {
  LoadingComponent,
 }) {
  let preloaded = false
-  const visibilityHandlers = []


Why did you remove this? It's to handle the case where preload is called after the component is part of the tree. In that scenario, we want all of the components to hit the underlying LoadableComponent to handle the interaction accordingly.

I believe by removing this and if preload is called after the component is mounted, then there will be a bug as it will fall back to the IntersectionObserver instead of already being loaded.

Of course, tests for this behaviour would be helpful 😅

You're right, I think I was confused by the fact that the this.state.visible is set based on the preloaded var in the constructor, so I assumed that this was unnecessary.

I added the visibilityHandlers back in and wrote a test that demonstrates that the underlying component will be displayed when preload is called after mount.

tazsingh

Almost there 🙂

tazsingh · 2018-06-18T23:01:01Z

src/__tests__/loadable-components/intersection-observer.test.js

  test('preload calls loadable load', () => {
    loadableVisiblity(opts).load()

    expect(loadable().load).toHaveBeenCalled()
  })

+  test('preload will cause the loadable component do be displayed', () => {


Thanks for adding this test! Can you add it for the react-loadable section as well?

Also minor typo: should be to instead of do

tazsingh · 2018-06-18T23:02:17Z

src/createLoadableVisibilityComponent.js

@@ -2,7 +2,7 @@ import React, { Component } from 'react'
 import { IntersectionObserver } from './capacities'

 let intersectionObserver
-let trackedElements = new Map()
+const trackedElements = new Map()


Can this now be removed as it's handled inside the mocked IntersectionObserver?

I'm not sure I follow—what does the mocked IntersectionObserver have to do with the trackedElements in createLoadableVisibilityComponent?

Sorry I confused this with something else. Took another look and all is good 👍

tazsingh · 2018-06-18T23:05:44Z

src/__mocks__/IntersectionObserver.js

@@ -1,4 +1,7 @@
 const intersectionObservers = []
+const trackedElements = []
+
+module.exports.trackedElements = trackedElements


I think the naming here is a bit confusing. Maybe allTrackedElements or globallyTrackedElements or similar to signify that it's a global that affects all instances of IntersectionObserver?

tazsingh · 2018-06-18T23:10:04Z

src/__mocks__/IntersectionObserver.js

@@ -17,7 +20,7 @@ module.exports.IntersectionObserver = class IntersectionObserver {
  constructor(callback) {
    this.callback = callback

-    this.trackedElements = []
+    this.trackedElements = trackedElements


I'm not a fan of initializing the IntersectionObserver with what was potentially previously tracked as it's not quite per spec. But I do like what you've done inside the LoadableVisibilityComponent itself where you double observed the tracked element via the IntersectionObserver and in trackedElements.

Perhaps a different approach would be something like:

class IntersectionObserver { constructor(callback) { // ... this.trackedElements = [] // ... } observe(element) { this.trackedElements.push(element) globallyTrackedElements.push(element) } unobserve(element) { // remove the element from this.trackedElements and globallyTrackedElements } }

Then you can assert against globallyTrackedElements to ensure that there aren't any leaks 👍

tazsingh · 2018-06-20T23:55:16Z

Released under 2.5.0 - https://github.com/stratiformltd/react-loadable-visibility/releases/tag/v2.5.0
Thanks!

set up observers in componentDidMount instead of constructor

6b0342f

tazsingh requested changes Jun 17, 2018

View reviewed changes

the83 force-pushed the th/fix-memory-leak branch from aedb8b6 to d5e5089 Compare June 18, 2018 16:38

PR feedback on memory leak fix

a84a0c9

the83 force-pushed the th/fix-memory-leak branch from d5e5089 to a84a0c9 Compare June 18, 2018 16:40

tazsingh requested changes Jun 18, 2018

View reviewed changes

more PR feedback on memory leak fix

1af8c78

tazsingh approved these changes Jun 20, 2018

View reviewed changes

tazsingh merged commit d951b11 into stratiformltd:master Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak when using server-side #11

Fix memory leak when using server-side #11

the83 commented Jun 1, 2018

tazsingh left a comment

tazsingh Jun 17, 2018

the83 Jun 18, 2018

tazsingh Jun 17, 2018

the83 Jun 18, 2018

tazsingh Jun 17, 2018

the83 Jun 18, 2018

tazsingh Jun 17, 2018

the83 Jun 18, 2018

tazsingh left a comment

tazsingh Jun 18, 2018

tazsingh Jun 18, 2018

the83 Jun 19, 2018

tazsingh Jun 20, 2018

tazsingh Jun 18, 2018

tazsingh Jun 18, 2018

tazsingh commented Jun 20, 2018

		@@ -1,8 +1,8 @@
		import React, { Component } from 'react'
		import * as React from 'react'

Fix memory leak when using server-side #11

Fix memory leak when using server-side #11

Conversation

the83 commented Jun 1, 2018

tazsingh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tazsingh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tazsingh commented Jun 20, 2018