fix: referenceNode memory leak in Manager (#312) (#313)

* fix: referenceNode memory leak

Instead of storing referenceNode in state, store it in a class property. This is done to enable the component to cleanup referenceNode’s value during unmounting.

I also added unit tests to account for this change, but that resulted in me having to update some dependencies and snapshots to get unit tests to pass.

* CR: Create separate setter and getter contexts

In order to help reduce re-renders of the Reference component, this commit creates separate context objects for the setters and getters of the referenceNode attribute.

This also removes the need for a context object, so it simplifies the management of the referenceNode attribute in the Manager class.

* CR Feedback: remove need for componentWillUnmount

* FIx flow

* Update size snapshot

Author: linusthe3rd

.size-snapshot.json

@@ -1,25 +1,25 @@
 {
   "dist/index.umd.js": {
-    "bundled": 67132,
-    "minified": 23802,
-    "gzipped": 7191
+    "bundled": 67000,
+    "minified": 23875,
+    "gzipped": 7202
   },
   "dist/index.umd.min.js": {
-    "bundled": 31768,
-    "minified": 12924,
-    "gzipped": 4205
+    "bundled": 31451,
+    "minified": 12925,
+    "gzipped": 4201
   },
   "dist/index.esm.js": {
-    "bundled": 12577,
-    "minified": 7364,
-    "gzipped": 2096,
+    "bundled": 12172,
+    "minified": 7049,
+    "gzipped": 2064,
     "treeshaked": {
       "rollup": {
-        "code": 3832,
+        "code": 3742,
         "import_statements": 137
       },
       "webpack": {
-        "code": 4938
+        "code": 4872
       }
     }
   }

src/Manager.js

@@ -2,50 +2,32 @@
 import * as React from 'react';
 import createContext, { type Context } from 'create-react-context';
 
-export const ManagerContext: Context<{
-  setReferenceNode?: (?HTMLElement) => void,
-  referenceNode?: ?HTMLElement,
-}> = createContext({ setReferenceNode: undefined, referenceNode: undefined });
+export const ManagerReferenceNodeContext: Context<?HTMLElement> = createContext();
+export const ManagerReferenceNodeSetterContext: Context<void | (?HTMLElement) => void> = createContext();
 
 export type ManagerProps = {
   children: React.Node,
 };
-type ManagerState = {
-  context: {
-    setReferenceNode?: (?HTMLElement) => void,
-    referenceNode?: ?HTMLElement,
-  },
-};
 
-export default class Manager extends React.Component<
-  ManagerProps,
-  ManagerState
-> {
-  constructor() {
-    super();
-    this.state = {
-      context: {
-        setReferenceNode: this.setReferenceNode,
-        referenceNode: undefined,
-      },
-    };
-  }
+export default class Manager extends React.Component<ManagerProps> {
+  referenceNode: ?HTMLElement;
 
-  setReferenceNode = (referenceNode: ?HTMLElement) => {
-    if (!referenceNode || this.state.context.referenceNode === referenceNode) {
-      return;
+  setReferenceNode = (newReferenceNode: ?HTMLElement) => {
+    if (this.referenceNode !== newReferenceNode) {
+      this.referenceNode = newReferenceNode;
+      this.forceUpdate();
     }
-
-    this.setState(({ context }) => ({
-      context: { ...context, referenceNode },
-    }));
   };
 
   render() {
     return (
-      <ManagerContext.Provider value={this.state.context}>
-        {this.props.children}
-      </ManagerContext.Provider>
+      <ManagerReferenceNodeContext.Provider value={this.referenceNode}>
+        <ManagerReferenceNodeSetterContext.Provider
+          value={this.setReferenceNode}
+        >
+          {this.props.children}
+        </ManagerReferenceNodeSetterContext.Provider>
+      </ManagerReferenceNodeContext.Provider>
     );
   }
 }

src/Manager.test.js

@@ -8,7 +8,7 @@ import { Manager, Popper, Reference } from '.';
 import { InnerPopper } from './Popper';
 
 // Private API
-import { ManagerContext } from './Manager';
+import { ManagerReferenceNodeContext, ManagerReferenceNodeSetterContext } from './Manager';
 
 describe('Manager component', () => {
   it('renders the expected markup', () => {
@@ -27,14 +27,18 @@ describe('Manager component', () => {
 
     const wrapper = mount(
       <Manager>
-        <ManagerContext.Consumer>
-          {({ setReferenceNode, referenceNode }) => (
-            <Reference
-              setReferenceNode={setReferenceNode}
-              referenceNode={referenceNode}
-            />
+        <ManagerReferenceNodeSetterContext.Consumer>
+          {(setReferenceNode) => (
+            <ManagerReferenceNodeContext.Consumer>
+              {(referenceNode) => (
+                <Reference
+                  setReferenceNode={setReferenceNode}
+                  referenceNode={referenceNode}
+                />
+              )}
+            </ManagerReferenceNodeContext.Consumer>
           )}
-        </ManagerContext.Consumer>
+        </ManagerReferenceNodeSetterContext.Consumer>
       </Manager>
     );
 
@@ -79,21 +83,51 @@ describe('Managed Reference', () => {
     const PopperInstance = wrapper.find(InnerPopper);
     expect(PopperInstance.prop('referenceElement')).toBe(referenceElement);
   });
+
+  it('updates the referenceNode if setReferenceNode is called with a new value', () => {
+    let referenceElement;
+    let ReferenceComp = ({ innerRef }) => (
+      <div
+        ref={node => {
+          // We just want to invoke this once so that we have access to the referenceElement in the upper scope.
+          if (referenceElement) return;
+          innerRef(node);
+          referenceElement = node;
+        }}
+      >
+        hello
+      </div>
+    );
+    const wrapper = mount(
+      <Manager>
+        <Reference>{({ ref }) => <ReferenceComp innerRef={ref} />}</Reference>
+        <Popper referenceElement={undefined}>{() => null}</Popper>
+      </Manager>
+    );
+
+    expect(wrapper.instance().referenceNode).toBe(referenceElement);
+    wrapper.instance().setReferenceNode(null);
+    expect(wrapper.instance().referenceNode).toBeNull();
+  });
 });
 
 describe('ReferenceNodeContext', () => {
   it('provides proper default values', () => {
     const Reference = () => null;
     const wrapper = mount(
       <div>
-        <ManagerContext.Consumer>
-          {({ setReferenceNode, referenceNode }) => (
-            <Reference
-              setReferenceNode={setReferenceNode}
-              referenceNode={referenceNode}
-            />
+        <ManagerReferenceNodeSetterContext.Consumer>
+          {(setReferenceNode) => (
+            <ManagerReferenceNodeContext.Consumer>
+              {(referenceNode) => (
+                <Reference
+                  setReferenceNode={setReferenceNode}
+                  referenceNode={referenceNode}
+                />
+              )}
+            </ManagerReferenceNodeContext.Consumer>
           )}
-        </ManagerContext.Consumer>
+        </ManagerReferenceNodeSetterContext.Consumer>
       </div>
     );
 

src/Popper.js

@@ -8,7 +8,7 @@ import PopperJS, {
   type ReferenceObject,
 } from 'popper.js';
 import type { Style } from 'typed-styles';
-import { ManagerContext } from './Manager';
+import { ManagerReferenceNodeContext } from './Manager';
 import { unwrapArray, setRef, shallowEqual } from './utils';
 import { type Ref } from "./RefTypes";
 
@@ -224,15 +224,15 @@ export { placements };
 
 export default function Popper({ referenceElement, ...props }: PopperProps) {
   return (
-    <ManagerContext.Consumer>
-      {({ referenceNode }) => (
+    <ManagerReferenceNodeContext.Consumer>
+      {(referenceNode) => (
         <InnerPopper
           referenceElement={
             referenceElement !== undefined ? referenceElement : referenceNode
           }
           {...props}
         />
       )}
-    </ManagerContext.Consumer>
+    </ManagerReferenceNodeContext.Consumer>
   );
 }

src/Reference.js

@@ -1,7 +1,7 @@
 // @flow
 import * as React from 'react';
 import warning from 'warning';
-import { ManagerContext } from './Manager';
+import { ManagerReferenceNodeSetterContext } from './Manager';
 import { safeInvoke, unwrapArray, setRef } from './utils';
 import { type Ref } from "./RefTypes";
 
@@ -38,10 +38,10 @@ class InnerReference extends React.Component<
 
 export default function Reference(props: ReferenceProps) {
   return (
-    <ManagerContext.Consumer>
-      {({ setReferenceNode }) => (
+    <ManagerReferenceNodeSetterContext.Consumer>
+      {(setReferenceNode) => (
         <InnerReference setReferenceNode={setReferenceNode} {...props} />
       )}
-    </ManagerContext.Consumer>
+    </ManagerReferenceNodeSetterContext.Consumer>
   );
 }

src/Reference.test.js

@@ -7,7 +7,7 @@ import { mount } from 'enzyme';
 import { Reference } from '.';
 
 // Private API
-import { ManagerContext } from './Manager';
+import { ManagerReferenceNodeSetterContext } from './Manager';
 
 jest.mock('warning');
 
@@ -22,33 +22,27 @@ describe('Arrow component', () => {
     // HACK: wrapping DIV needed to make Enzyme happy for now
     const wrapper = mount(
       <div>
-        <ManagerContext.Provider
-          value={{
-            setReferenceNode,
-            referenceNode: undefined,
-          }}
+        <ManagerReferenceNodeSetterContext.Provider
+          value={setReferenceNode}
         >
           <Reference>{({ ref }) => <div ref={ref} />}</Reference>
-        </ManagerContext.Provider>
+        </ManagerReferenceNodeSetterContext.Provider>
       </div>
     );
     expect(wrapper.children()).toMatchSnapshot();
   });
 
-  it('consumes the ManagerContext from Manager', () => {
+  it('consumes the ManagerReferenceNodeSetterContext from Manager', () => {
     const setReferenceNode = jest.fn();
 
     // HACK: wrapping DIV needed to make Enzyme happy for now
     mount(
       <div>
-        <ManagerContext.Provider
-          value={{
-            setReferenceNode,
-            referenceNode: undefined,
-          }}
+        <ManagerReferenceNodeSetterContext.Provider
+          value={setReferenceNode}
         >
           <Reference>{({ ref }) => <div ref={ref} />}</Reference>
-        </ManagerContext.Provider>
+        </ManagerReferenceNodeSetterContext.Provider>
       </div>
     );
     expect(setReferenceNode).toHaveBeenCalled();
@@ -60,14 +54,11 @@ describe('Arrow component', () => {
     // HACK: wrapping DIV needed to make Enzyme happy for now
     mount(
       <div>
-        <ManagerContext.Provider
-          value={{
-            setReferenceNode,
-            referenceNode: undefined,
-          }}
+        <ManagerReferenceNodeSetterContext.Provider
+          value={setReferenceNode}
         >
           <Reference>{({ ref }) => <div ref={ref} />}</Reference>
-        </ManagerContext.Provider>
+        </ManagerReferenceNodeSetterContext.Provider>
       </div>
     );
     expect(warning).toHaveBeenCalledWith(
@@ -80,13 +71,11 @@ describe('Arrow component', () => {
     // HACK: wrapping DIV needed to make Enzyme happy for now
     mount(
       <div>
-        <ManagerContext.Provider
-          value={{
-            referenceNode: undefined,
-          }}
+        <ManagerReferenceNodeSetterContext.Provider
+          value={undefined}
         >
           <Reference>{({ ref }) => <div ref={ref} />}</Reference>
-        </ManagerContext.Provider>
+        </ManagerReferenceNodeSetterContext.Provider>
       </div>
     );
     expect(warning).toHaveBeenCalledWith(