Max scroll perf

src/state/middleware/max-scroll-updater.js

@@ -1,32 +1,63 @@
 // @flow
 import invariant from 'tiny-invariant';
 import type { Position } from 'css-box-model';
-import type { State, Viewport } from '../../types';
+import type { State, Viewport, DraggableLocation } from '../../types';
 import type { Action, Store } from '../store-types';
 import getMaxScroll from '../get-max-scroll';
 import { isEqual } from '../position';
 import { updateViewportMaxScroll } from '../action-creators';
 import isMovementAllowed from '../is-movement-allowed';
 
-const shouldCheckMaxScroll = (action: Action): boolean =>
+const shouldCheckOnAction = (action: Action): boolean =>
   action.type === 'MOVE' ||
   action.type === 'MOVE_UP' ||
   action.type === 'MOVE_RIGHT' ||
   action.type === 'MOVE_DOWN' ||
   action.type === 'MOVE_LEFT' ||
   action.type === 'MOVE_BY_WINDOW_SCROLL';
 
-const getNewMaxScroll = (state: State, action: Action): ?Position => {
-  if (!isMovementAllowed(state)) {
+// optimisation: body size can only change when the destination has changed
+const hasDroppableOverChanged = (
+  previous: ?DraggableLocation,
+  current: ?DraggableLocation,
+): boolean => {
+  // no previous - if there is a next return true
+  if (!previous) {
+    return Boolean(current);
+  }
+
+  // no current - if there is a previous return true
+  if (!current) {
+    return Boolean(previous);
+  }
+
+  return previous.droppableId !== current.droppableId;
+};
+
+const getNewMaxScroll = (
+  previous: State,
+  current: State,
+  action: Action,
+): ?Position => {
+  if (!shouldCheckOnAction(action)) {
     return null;
   }
 
-  if (!shouldCheckMaxScroll(action)) {
+  if (!isMovementAllowed(previous) || !isMovementAllowed(current)) {
+    return null;
+  }
+
+  if (
+    !hasDroppableOverChanged(
+      previous.impact.destination,
+      current.impact.destination,
+    )
+  ) {
     return null;
   }
 
   // check to see if the viewport max scroll has changed
-  const viewport: Viewport = state.viewport;
+  const viewport: Viewport = current.viewport;
 
   const doc: ?HTMLElement = document.documentElement;
   invariant(doc, 'Could not find document.documentElement');
@@ -51,11 +82,13 @@ const getNewMaxScroll = (state: State, action: Action): ?Position => {
 export default (store: Store) => (next: Action => mixed) => (
   action: Action,
 ): mixed => {
-  const maxScroll: ?Position = getNewMaxScroll(store.getState(), action);
+  const previous: State = store.getState();
+  next(action);
+  const current: State = store.getState();
+  const maxScroll: ?Position = getNewMaxScroll(previous, current, action);
+
   // max scroll has changed - updating before action
   if (maxScroll) {
     next(updateViewportMaxScroll(maxScroll));
   }
-
-  next(action);
 };

test/unit/state/middleware/max-scroll-updater.spec.js

@@ -9,6 +9,7 @@ import {
   updateViewportMaxScroll,
   initialPublish,
   moveDown,
+  moveRight,
 } from '../../../../src/state/action-creators';
 import type { Store } from '../../../../src/state/store-types';
 import type { Viewport } from '../../../../src/types';
@@ -39,59 +40,81 @@ afterAll(() => {
   doc.scrollWidth = originalWidth;
 });
 
-it('should not update the max viewport scroll if no drag is occurring', () => {
+describe('not dragging', () => {
+  it('should not update the max viewport scroll if no drag is occurring', () => {
+    const mock = jest.fn();
+    const store: Store = createStore(middleware, passThrough(mock));
+
+    doc.scrollHeight = scrollHeight + 10;
+    doc.scrollWidth = scrollWidth + 10;
+
+    store.dispatch(prepare());
+
+    expect(mock).toHaveBeenCalledTimes(1);
+    expect(mock).toHaveBeenCalledWith(prepare());
+  });
+});
+
+it('should update if the max scroll position has changed and the destination has changed', () => {
   const mock = jest.fn();
   const store: Store = createStore(middleware, passThrough(mock));
 
+  // now dragging
+  store.dispatch(prepare());
+  store.dispatch(initialPublish(initialPublishArgs));
+  expect(store.getState().isDragging).toBe(true);
+  mock.mockClear();
+
+  // change in scroll size
   doc.scrollHeight = scrollHeight + 10;
   doc.scrollWidth = scrollWidth + 10;
 
-  store.dispatch(prepare());
-
-  expect(mock).toHaveBeenCalledTimes(1);
-  expect(mock).toHaveBeenCalledWith(prepare());
+  const expected: Position = getMaxScroll({
+    height: viewport.frame.height,
+    width: viewport.frame.width,
+    scrollHeight: scrollHeight + 10,
+    scrollWidth: scrollWidth + 10,
+  });
+  // changing droppable
+  store.dispatch(moveRight());
+  expect(mock).toHaveBeenCalledTimes(2);
+  expect(mock).toHaveBeenCalledWith(moveRight());
+  expect(mock).toHaveBeenCalledWith(updateViewportMaxScroll(expected));
 });
 
-describe('during a drag', () => {
-  it('should not update the max viewport scroll if the max scroll position has not changed', () => {
-    const mock = jest.fn();
-    const store: Store = createStore(middleware, passThrough(mock));
+it('should not update if the max scroll position has not changed and destination has', () => {
+  const mock = jest.fn();
+  const store: Store = createStore(middleware, passThrough(mock));
 
-    // now dragging
-    store.dispatch(prepare());
-    store.dispatch(initialPublish(initialPublishArgs));
-    expect(store.getState().isDragging).toBe(true);
-    mock.mockClear();
+  // now dragging
+  store.dispatch(prepare());
+  store.dispatch(initialPublish(initialPublishArgs));
+  expect(store.getState().isDragging).toBe(true);
+  mock.mockClear();
 
-    // no change in scroll size
-    store.dispatch(moveDown());
-    expect(mock).toHaveBeenCalledWith(moveDown());
-    expect(mock).toHaveBeenCalledTimes(1);
-  });
+  // no change in scroll size but there is a change in destination
+  store.dispatch(moveRight());
+  expect(mock).toHaveBeenCalledWith(moveRight());
+  expect(mock).toHaveBeenCalledTimes(1);
+});
 
-  it('should update the max viewport scroll if the max scroll position has changed', () => {
-    const mock = jest.fn();
-    const store: Store = createStore(middleware, passThrough(mock));
+it('should not update if the destination has not changed (even if the scroll size has changed)', () => {
+  // the scroll size should not change in response to a drag if the destination has not changed
+  const mock = jest.fn();
+  const store: Store = createStore(middleware, passThrough(mock));
 
-    // now dragging
-    store.dispatch(prepare());
-    store.dispatch(initialPublish(initialPublishArgs));
-    expect(store.getState().isDragging).toBe(true);
-    mock.mockClear();
+  // now dragging
+  store.dispatch(prepare());
+  store.dispatch(initialPublish(initialPublishArgs));
+  expect(store.getState().isDragging).toBe(true);
+  mock.mockClear();
 
-    // change in scroll size
-    doc.scrollHeight = scrollHeight + 10;
-    doc.scrollWidth = scrollWidth + 10;
+  // change in scroll size
+  doc.scrollHeight = scrollHeight + 10;
+  doc.scrollWidth = scrollWidth + 10;
 
-    const expected: Position = getMaxScroll({
-      height: viewport.frame.height,
-      width: viewport.frame.width,
-      scrollHeight: scrollHeight + 10,
-      scrollWidth: scrollWidth + 10,
-    });
-    store.dispatch(moveDown());
-    expect(mock).toHaveBeenCalledTimes(2);
-    expect(mock).toHaveBeenCalledWith(moveDown());
-    expect(mock).toHaveBeenCalledWith(updateViewportMaxScroll(expected));
-  });
+  // not changing droppable
+  store.dispatch(moveDown());
+  expect(mock).toHaveBeenCalledTimes(1);
+  expect(mock).toHaveBeenCalledWith(moveDown());
 });