Skip to content

Commit

Permalink
Moveable perf fix (#566)
Browse files Browse the repository at this point in the history
  • Loading branch information
alexreardon authored Jun 26, 2018
1 parent e16642c commit f31df80
Show file tree
Hide file tree
Showing 18 changed files with 246 additions and 240 deletions.
10 changes: 10 additions & 0 deletions src/debug/timings.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,19 @@ const records: Records = {};

const flag: string = '__react-beautiful-dnd-debug-timings-hook__';

// we want to strip all the code out for production builds
// draw back: can only do timings in dev env (which seems to be fine for now)
const isProduction: boolean = process.env.NODE_ENV === 'production';

const isTimingsEnabled = (): boolean => Boolean(window[flag]);

// Debug: uncomment to enable
// window[flag] = true;

export const start = (key: string) => {
if (isProduction) {
return;
}
if (!isTimingsEnabled()) {
return;
}
Expand All @@ -29,6 +36,9 @@ type Style = {|
|}

export const finish = (key: string) => {
if (isProduction) {
return;
}
if (!isTimingsEnabled()) {
return;
}
Expand Down
6 changes: 2 additions & 4 deletions src/state/auto-scroller/jump-scroller.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @flow
import invariant from 'tiny-invariant';
import { type Position } from 'css-box-model';
import { add, subtract } from '../position';
import {
Expand Down Expand Up @@ -93,10 +94,7 @@ export default ({

const destination: ?DraggableLocation = state.impact.destination;

if (!destination) {
console.error('Cannot perform a jump scroll when there is no destination');
return;
}
invariant(destination, 'Cannot perform a jump scroll when there is no destination');

// 1. We scroll the droppable first if we can to avoid the draggable
// leaving the list
Expand Down
5 changes: 1 addition & 4 deletions src/view/drag-handle/drag-handle.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,7 @@ export default class DragHandle extends Component<Props> {
// storing a reference for later
this.lastDraggableRef = draggableRef;

if (!draggableRef) {
console.error('Cannot get draggable ref from drag handle');
return;
}
invariant(draggableRef, 'Cannot get draggable ref from drag handle');

// drag handle ref will not be available when not enabled
if (!this.props.isEnabled) {
Expand Down
7 changes: 3 additions & 4 deletions src/view/drag-handle/sensor/create-mouse-sensor.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,12 @@ export default ({
return;
}

// drag should be pending
if (!state.pending) {
console.error('invalid state');
return;
kill();
invariant(false, 'Expected there to be a pending drag');
}

// drag is pending

// threshold not yet exceeded
if (!isSloppyClickThresholdExceeded(state.pending, point)) {
return;
Expand Down
3 changes: 1 addition & 2 deletions src/view/drag-handle/sensor/create-touch-sensor.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,8 @@ export default ({
const pending: ?Position = state.pending;

if (!pending) {
console.error('cannot start a touch drag without a pending position');
kill();
return;
invariant(false, 'cannot start a touch drag without a pending position');
}

setState({
Expand Down
6 changes: 2 additions & 4 deletions src/view/drag-handle/util/create-event-marshal.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @flow
import invariant from 'tiny-invariant';

export type EventMarshal = {|
handle: () => void,
Expand All @@ -10,10 +11,7 @@ export default (): EventMarshal => {
let isMouseDownHandled: boolean = false;

const handle = (): void => {
if (isMouseDownHandled) {
console.error('Cannot handle mouse down as it is already handled');
return;
}
invariant(!isMouseDownHandled, 'Cannot handle mouse down as it is already handled');
isMouseDownHandled = true;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ export default class DraggableDimensionPublisher extends Component<Props> {
}

unpublish = () => {
if (!this.publishedDescriptor) {
console.error('cannot unpublish descriptor when none is published');
return;
}
invariant(this.publishedDescriptor, 'Cannot unpublish descriptor when none is published')

// Using the previously published id to unpublish. This is to guard
// against the case where the id dynamically changes. This is not
Expand Down
17 changes: 16 additions & 1 deletion src/view/draggable/connected-draggable.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { connect } from 'react-redux';
import Draggable from './draggable';
import { storeKey } from '../context-keys';
import { negate } from '../../state/position';
import isStrictEqual from '../is-strict-equal';
import getDisplacementMap, { type DisplacementMap } from '../../state/get-displacement-map';
import {
lift as liftAction,
Expand Down Expand Up @@ -219,10 +220,24 @@ const mapDispatchToProps: DispatchProps = {
// that `connect` provides.
// It avoids needing to do it own within `Draggable`
const ConnectedDraggable: OwnProps => Node = (connect(
// returning a function so each component can do its own memoization
makeMapStateToProps,
(mapDispatchToProps: any),
// mergeProps: use default
null,
{ storeKey },
// options
{
// Using our own store key.
// This allows consumers to also use redux
// Note: the default store key is 'store'
storeKey,
// Default value, but being really clear
pure: true,
// When pure, compares the result of mapStateToProps to its previous value.
// Default value: shallowEqual
// Switching to a strictEqual as we return a memoized object on changes
areStatePropsEqual: isStrictEqual,
},
): any)(Draggable);

ConnectedDraggable.defaultProps = ({
Expand Down
88 changes: 46 additions & 42 deletions src/view/draggable/draggable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { type Position, type BoxModel } from 'css-box-model';
import PropTypes from 'prop-types';
import memoizeOne from 'memoize-one';
import invariant from 'tiny-invariant';
import { isEqual } from '../../state/position';
import type {
DraggableDimension,
ItemPositions,
Expand Down Expand Up @@ -33,7 +34,8 @@ import type {
ZIndexOptions,
} from './draggable-types';
import getWindowScroll from '../window/get-window-scroll';
import type { Speed, Style as MovementStyle } from '../moveable/moveable-types';
import throwIfRefIsInvalid from '../throw-if-invalid-inner-ref';
import type { Speed } from '../moveable/moveable-types';

export const zIndexOptions: ZIndexOptions = {
dragging: 5000,
Expand All @@ -42,6 +44,15 @@ export const zIndexOptions: ZIndexOptions = {

const origin: Position = { x: 0, y: 0 };

const getTranslate = (offset: Position): ?string => {
// we do not translate to origin
// we simply clear the translate
if (isEqual(offset, origin)) {
return null;
}
return `translate(${offset.x}px, ${offset.y}px)`;
};

export default class Draggable extends Component<Props> {
/* eslint-disable react/sort-comp */
callbacks: DragHandleCallbacks
Expand Down Expand Up @@ -76,12 +87,7 @@ export default class Draggable extends Component<Props> {
}

componentDidMount() {
if (!this.ref) {
console.error(`
Draggable has not been provided with a ref.
Please use the DraggableProvided > innerRef function
`);
}
throwIfRefIsInvalid(this.ref);
}

componentWillUnmount() {
Expand All @@ -92,10 +98,6 @@ export default class Draggable extends Component<Props> {
// This should already be handled gracefully in DragHandle.
// Just being extra clear here
throwIfCannotDrag() {
invariant(this.ref, `
Draggable: cannot drag as no DOM node has been provided
Please ensure you provide a DOM node using the DraggableProvided > innerRef function
`);
invariant(!this.props.isDragDisabled,
'Draggable: cannot drag as dragging is not enabled'
);
Expand All @@ -113,8 +115,8 @@ export default class Draggable extends Component<Props> {
const { clientSelection, autoScrollMode } = options;
const { lift, draggableId } = this.props;
const ref: ?HTMLElement = this.ref;

invariant(ref, 'Cannot lift at this time as there is no ref');
throwIfRefIsInvalid(ref);
invariant(ref);

const client: ItemPositions = {
selection: clientSelection,
Expand Down Expand Up @@ -194,16 +196,18 @@ export default class Draggable extends Component<Props> {
// At this point the ref has been changed or initially populated

this.ref = ref;
throwIfRefIsInvalid(ref);
})

getDraggableRef = (): ?HTMLElement => this.ref;

getDraggingStyle = memoizeOne(
(dimension: DraggableDimension,
(
change: Position,
dimension: DraggableDimension,
isDropAnimating: boolean,
movementStyle: MovementStyle): DraggingStyle => {
): DraggingStyle => {
const box: BoxModel = dimension.client;

const style: DraggingStyle = {
// ## Placement
position: 'fixed',
Expand All @@ -223,7 +227,7 @@ export default class Draggable extends Component<Props> {
// Layering
zIndex: isDropAnimating ? zIndexOptions.dropAnimating : zIndexOptions.dragging,
// Moving in response to user input
transform: movementStyle.transform ? `${movementStyle.transform}` : null,
transform: getTranslate(change),

// ## Performance
pointerEvents: 'none',
Expand All @@ -233,9 +237,9 @@ export default class Draggable extends Component<Props> {
)

getNotDraggingStyle = memoizeOne(
(movementStyle: MovementStyle, shouldAnimateDisplacement: boolean): NotDraggingStyle => {
(current: Position, shouldAnimateDisplacement: boolean): NotDraggingStyle => {
const style: NotDraggingStyle = {
transform: movementStyle.transform,
transform: getTranslate(current),
// use the global animation for animation - or opt out of it
transition: shouldAnimateDisplacement ? null : 'none',
// transition: css.outOfTheWay,
Expand All @@ -246,25 +250,25 @@ export default class Draggable extends Component<Props> {

getProvided = memoizeOne(
(
change: Position,
isDragging: boolean,
isDropAnimating: boolean,
shouldAnimateDisplacement: boolean,
dimension: ?DraggableDimension,
dragHandleProps: ?DragHandleProps,
movementStyle: MovementStyle,
): Provided => {
const useDraggingStyle: boolean = isDragging || isDropAnimating;

const draggableStyle: DraggableStyle = (() => {
if (!useDraggingStyle) {
return this.getNotDraggingStyle(movementStyle, shouldAnimateDisplacement);
return this.getNotDraggingStyle(change, shouldAnimateDisplacement);
}

invariant(dimension, 'draggable dimension required for dragging');

// Need to position element in original visual position. To do this
// we position it without
return this.getDraggingStyle(dimension, isDropAnimating, movementStyle);
return this.getDraggingStyle(change, dimension, isDropAnimating);
})();

const provided: Provided = {
Expand All @@ -288,22 +292,25 @@ export default class Draggable extends Component<Props> {
draggingOver,
}))

getSpeed = memoizeOne(
(isDragging: boolean, shouldAnimateDragMovement: boolean, isDropAnimating: boolean): Speed => {
if (isDropAnimating) {
return 'STANDARD';
}
getSpeed = (
isDragging: boolean,
shouldAnimateDragMovement: boolean,
isDropAnimating: boolean
): Speed => {
if (isDropAnimating) {
return 'STANDARD';
}

// if dragging and can animate - then move quickly
if (isDragging && shouldAnimateDragMovement) {
return 'FAST';
}
if (isDragging && shouldAnimateDragMovement) {
return 'FAST';
}

// Animation taken care of by css
return 'INSTANT';
})
// if dragging: no animation
// if not dragging: animation done with CSS
return 'INSTANT';
}

renderChildren = (movementStyle: MovementStyle, dragHandleProps: ?DragHandleProps): ?Node => {
renderChildren = (change: Position, dragHandleProps: ?DragHandleProps): ?Node => {
const {
isDragging,
isDropAnimating,
Expand All @@ -315,12 +322,12 @@ export default class Draggable extends Component<Props> {

const child: ?Node = children(
this.getProvided(
change,
isDragging,
isDropAnimating,
shouldAnimateDisplacement,
dimension,
dragHandleProps,
movementStyle,
),
this.getSnapshot(
isDragging,
Expand All @@ -336,10 +343,7 @@ export default class Draggable extends Component<Props> {
return null;
}

if (!dimension) {
console.error('Draggable: Dimension is required for dragging');
return null;
}
invariant(dimension, 'Draggable: Dimension is required for dragging');

return <Placeholder placeholder={dimension.placeholder} />;
})();
Expand Down Expand Up @@ -386,7 +390,7 @@ export default class Draggable extends Component<Props> {
destination={offset}
onMoveEnd={this.onMoveEnd}
>
{(movementStyle: MovementStyle) => (
{(change: Position) => (
<DragHandle
draggableId={draggableId}
isDragging={isDragging}
Expand All @@ -398,7 +402,7 @@ export default class Draggable extends Component<Props> {
canDragInteractiveElements={disableInteractiveElementBlocking}
>
{(dragHandleProps: ?DragHandleProps) =>
this.renderChildren(movementStyle, dragHandleProps)
this.renderChildren(change, dragHandleProps)
}
</DragHandle>
)}
Expand Down
Loading

0 comments on commit f31df80

Please sign in to comment.