feat: allow popper modfiers to be updated (#302)

* Allow popper modfiers to be updated

Currently if a consumer updates the `modifiers` object prop, unless another prop also changes, the new modifiers are not applied. For example, if the passed modifier `flip.enabled` were to change from `true` to `false`, the popper doesn't pick up on that fact and continues allowing the popover contents to flip.

This PR allows for the `Popper` component to construct a new popper instance with the new modifiers if and only if the `modifiers` object changes (note: this is a reference comparison and not a value comparison).

I'm not that familiar with Popper.JS so if there is a more efficient way to make sure modifier changes get propagated to the underlying popover, please let me know!

As it stands, this _could_ cause some unnecessary popper instance creation/destruction if the consumer isn't properly memoizing the `modifiers` object they pass. This could be partially solved by doing something like shallow comparing each modifier option object one by one instead of a top level reference comparison. Alternatively, if there is a more performant way to imperatively update an existing popper instance, that may be better to use to avoid as much churn. In fact, it would be even possible to combine both of these solutions as well.

* Added development warning if not memoizing

* fix tests

* Extracted shallow equals to a utility

* Updated based on feedback

Author: maclockard

.size-snapshot.json

@@ -1,25 +1,25 @@
 {
   "dist/index.umd.js": {
-    "bundled": 65802,
-    "minified": 23248,
-    "gzipped": 7012
+    "bundled": 66802,
+    "minified": 23724,
+    "gzipped": 7169
   },
   "dist/index.umd.min.js": {
-    "bundled": 31389,
-    "minified": 12810,
-    "gzipped": 4174
+    "bundled": 31438,
+    "minified": 12846,
+    "gzipped": 4182
   },
   "dist/index.esm.js": {
-    "bundled": 11275,
-    "minified": 6748,
-    "gzipped": 1854,
+    "bundled": 12271,
+    "minified": 7288,
+    "gzipped": 2066,
     "treeshaked": {
       "rollup": {
-        "code": 3718,
+        "code": 3754,
         "import_statements": 137
       },
       "webpack": {
-        "code": 4824
+        "code": 4860
       }
     }
   }

demo/index.js

@@ -53,6 +53,16 @@ const modifiers = {
   hide: { enabled: false },
 };
 
+const animatedModifiers = {
+  ...modifiers,
+  // We disable the built-in gpuAcceleration so that
+  // Popper.js will return us easy to interpolate values
+  // (top, left instead of transform: translate3d)
+  // We'll then use these values to generate the needed
+  // css tranform values blended with the react-spring values
+  computeStyle: { gpuAcceleration: false },
+}
+
 const Demo = enhance(
   ({ activePlacement, setActivePlacement, isPopper2Open, togglePopper2 }) => (
     <Fragment>
@@ -121,15 +131,7 @@ const Demo = enhance(
               show ? ({ rotation, scale, opacity, top: topOffset }) => (
                   <Popper
                     placement="bottom"
-                    modifiers={{
-                      ...modifiers,
-                      // We disable the built-in gpuAcceleration so that
-                      // Popper.js will return us easy to interpolate values
-                      // (top, left instead of transform: translate3d)
-                      // We'll then use these values to generate the needed
-                      // css tranform values blended with the react-spring values
-                      computeStyle: { gpuAcceleration: false },
-                    }}
+                    modifiers={animatedModifiers}
                   >
                     {({
                       ref,

src/Popper.js

@@ -9,7 +9,7 @@ import PopperJS, {
 } from 'popper.js';
 import type { Style } from 'typed-styles';
 import { ManagerContext } from './Manager';
-import { safeInvoke, unwrapArray } from './utils';
+import { safeInvoke, unwrapArray, shallowEqual } from './utils';
 
 type getRefFn = (?HTMLElement) => void;
 type ReferenceElement = ReferenceObject | HTMLElement | null;
@@ -164,8 +164,22 @@ export class InnerPopper extends React.Component<PopperProps, PopperState> {
     if (
       this.props.placement !== prevProps.placement ||
       this.props.referenceElement !== prevProps.referenceElement ||
-      this.props.positionFixed !== prevProps.positionFixed
+      this.props.positionFixed !== prevProps.positionFixed ||
+      this.props.modifiers !== prevProps.modifiers
     ) {
+
+      // develop only check that modifiers isn't being updated needlessly
+      if (process.env.NODE_ENV === "development") {
+        if (
+          this.props.modifiers !== prevProps.modifiers &&
+          this.props.modifiers != null &&
+          prevProps.modifiers != null &&
+          shallowEqual(this.props.modifiers, prevProps.modifiers)
+        ) {
+          console.warn("'modifiers' prop reference updated even though all values appear the same.\nConsider memoizing the 'modifiers' object to avoid needless rendering.");
+        }
+      }
+
       this.updatePopperInstance();
     } else if (
       this.props.eventsEnabled !== prevProps.eventsEnabled &&

src/utils.js

@@ -15,3 +15,26 @@ export const safeInvoke = (fn: ?Function, ...args: *) => {
     return fn(...args);
   }
 }
+
+/**
+ * Does a shallow equality check of two objects by comparing the reference
+ * equality of each value.
+ */
+export const shallowEqual = (objA: { [key: string]: any}, objB: { [key: string]: any}): boolean => {
+  var aKeys = Object.keys(objA);
+  var bKeys = Object.keys(objB);
+
+  if (bKeys.length !== aKeys.length) {
+    return false;
+  }
+
+  for (var i = 0; i < bKeys.length; i++) {
+    var key = aKeys[i];
+
+    if (objA[key] !== objB[key]) {
+      return false;
+    }
+  }
+
+  return true;
+}