diff --git a/src/state/action-creators.js b/src/state/action-creators.js index ef6864cdd6..3c4f762d3f 100644 --- a/src/state/action-creators.js +++ b/src/state/action-creators.js @@ -55,14 +55,6 @@ export const requestDimensions = (type: TypeId): RequestDimensionsAction => ({ payload: type, }); -export type BeginLiftAction = {| - type: 'BEGIN_LIFT' -|} - -const beginLift = (): BeginLiftAction => ({ - type: 'BEGIN_LIFT', -}); - export type CompleteLiftAction = {| type: 'COMPLETE_LIFT', payload: {| @@ -74,7 +66,7 @@ export type CompleteLiftAction = {| |} |} -const completeLift = (id: DraggableId, +export const completeLift = (id: DraggableId, type: TypeId, client: InitialDragLocation, windowScroll: Position, @@ -233,6 +225,16 @@ export const clean = (): CleanAction => ({ payload: null, }); +type PrepareAction = { + type: 'PREPARE', + payload: null, +} + +export const prepare = (): PrepareAction => ({ + type: 'PREPARE', + payload: null, +}); + export type DropAnimateAction = { type: 'DROP_ANIMATE', payload: {| @@ -279,26 +281,21 @@ export const drop = () => (dispatch: Dispatch, getState: () => State): void => { const state: State = getState(); - // This can occur if the user ends a drag before - // the collecting phase is finished. - // This will not trigger a hook as the hook waits - // for a DRAGGING phase before firing a onDragStart + // dropped before a drag officially started - this is fine if (state.phase === 'COLLECTING_DIMENSIONS') { - console.error('canceling drag while collecting'); dispatch(clean()); return; } + // dropped in another phase except for dragging - this is an error if (state.phase !== 'DRAGGING') { - console.error('cannot drop if not dragging', state); - // We need to wrap this in a setTimeout to avoid a race - // condition with `lift`, which includes timeouts - setTimeout(() => dispatch(clean())); + console.error(`not able to drop in phase: '${state.phase}'`); + dispatch(clean()); return; } if (!state.drag) { - console.error('invalid drag state', state); + console.error('not able to drop when there is invalid drag state', state); dispatch(clean()); return; } @@ -427,55 +424,55 @@ export type LiftAction = {| |} |} -// using redux-thunk export const lift = (id: DraggableId, type: TypeId, client: InitialDragLocation, windowScroll: Position, isScrollAllowed: boolean, ) => (dispatch: Dispatch, getState: Function) => { - (() => { - const state: State = getState(); - // quickly finish any current animations - if (state.phase === 'DROP_ANIMATING') { - if (!state.drop || !state.drop.pending) { - console.error('cannot flush drop animation if there is no pending'); - dispatch(clean()); - return; - } - dispatch(completeDrop(state.drop.pending.result)); + // Phase 1: Quickly finish any current drop animations + const initial: State = getState(); + + if (initial.phase === 'DROP_ANIMATING') { + if (!initial.drop || !initial.drop.pending) { + console.error('cannot flush drop animation if there is no pending'); + dispatch(clean()); + } else { + dispatch(completeDrop(initial.drop.pending.result)); } - })(); + } // https://github.com/chenglou/react-motion/issues/437 // need to allow a flush of react-motion + dispatch(prepare()); + setTimeout(() => { + // Phase 2: collect all dimensions const state: State = getState(); - if (state.phase !== 'IDLE' && state.phase !== 'DRAG_COMPLETE') { - dispatch(clean()); + // drag cancelled before timeout finished + if (state.phase !== 'PREPARING') { + return; } - dispatch(beginLift()); dispatch(requestDimensions(type)); - // Dimensions will be requested synchronously - // after they are done - lift. - // Could improve this by explicitly waiting until all dimensions are published. - // Could also allow a lift to occur before all the dimensions are published + // Need to allow an opportunity for the dimensions to be requested. setTimeout(() => { + // Phase 3: dimensions are collected: start a lift const newState: State = getState(); // drag was already cancelled before dimensions all collected if (newState.phase !== 'COLLECTING_DIMENSIONS') { return; } + dispatch(completeLift(id, type, client, windowScroll, isScrollAllowed)); }); }); }; -export type Action = BeginLiftAction | +export type Action = CompleteLiftAction | RequestDimensionsAction | PublishDraggableDimensionAction | @@ -487,4 +484,5 @@ export type Action = BeginLiftAction | CrossAxisMoveBackwardAction | DropAnimateAction | DropCompleteAction | + PrepareAction | CleanAction; diff --git a/src/state/reducer.js b/src/state/reducer.js index c7eda64ed8..2ed5a756f3 100644 --- a/src/state/reducer.js +++ b/src/state/reducer.js @@ -37,17 +37,12 @@ const noDimensions: DimensionState = { const origin: Position = { x: 0, y: 0 }; -const clean = memoizeOne((phase: ?Phase): State => { - const state: State = { - // flow was not good with having a default arg on an optional type - phase: phase || 'IDLE', - drag: null, - drop: null, - dimension: noDimensions, - }; - - return state; -}); +const clean = memoizeOne((phase?: Phase = 'IDLE'): State => ({ + phase, + drag: null, + drop: null, + dimension: noDimensions, +})); type MoveArgs = {| state: State, @@ -135,18 +130,18 @@ const move = ({ }; export default (state: State = clean('IDLE'), action: Action): State => { - if (action.type === 'BEGIN_LIFT') { - if (state.phase !== 'IDLE') { - console.error('trying to start a lift while another is occurring'); - return state; - } - return clean('COLLECTING_DIMENSIONS'); + if (action.type === 'CLEAN') { + return clean(); + } + + if (action.type === 'PREPARE') { + return clean('PREPARING'); } if (action.type === 'REQUEST_DIMENSIONS') { - if (state.phase !== 'COLLECTING_DIMENSIONS') { - console.error('trying to collect dimensions at the wrong time'); - return state; + if (state.phase !== 'PREPARING') { + console.error('trying to start a lift while not preparing for a lift'); + return clean(); } const typeId: TypeId = action.payload; @@ -538,9 +533,5 @@ export default (state: State = clean('IDLE'), action: Action): State => { }; } - if (action.type === 'CLEAN') { - return clean(); - } - return state; }; diff --git a/src/types.js b/src/types.js index 92a2ffb893..81ff4a82b2 100644 --- a/src/types.js +++ b/src/types.js @@ -208,7 +208,30 @@ export type PendingDrop = {| result: DropResult, |} -export type Phase = 'IDLE' | 'COLLECTING_DIMENSIONS' | 'DRAGGING' | 'DROP_ANIMATING' | 'DROP_COMPLETE'; +export type Phase = + // The application rest state + 'IDLE' | + + // When a drag starts we need to flush any existing animations + // that might be occurring. While this flush is occurring we + // are in this phase + 'PREPARING' | + + // After the animations have been flushed we need to collect the + // dimensions of all of the Draggable and Droppable components. + // At this point a drag has not started yet and the onDragStart + // hook has not fired. + 'COLLECTING_DIMENSIONS' | + + // A drag is active. The onDragStart hook has been fired + 'DRAGGING' | + + // An optional phase for animating the drop / cancel if it is needed + 'DROP_ANIMATING' | + + // The final state of a drop / cancel. + // This will result in the onDragEnd hook being fired + 'DROP_COMPLETE'; export type DimensionState = {| request: ?TypeId, diff --git a/test/unit/state/action-creators.spec.js b/test/unit/state/action-creators.spec.js new file mode 100644 index 0000000000..dd7d4f7d01 --- /dev/null +++ b/test/unit/state/action-creators.spec.js @@ -0,0 +1,250 @@ +// @flow +import { + cancel, + clean, + lift, + completeDrop, + prepare, + completeLift, + requestDimensions, + publishDraggableDimension, + publishDroppableDimension, +} from '../../../src/state/action-creators'; +import createStore from '../../../src/state/create-store'; +import noImpact from '../../../src/state/no-impact'; +import { getPreset } from '../../utils/dimension'; +import type { + State, + Position, + DraggableId, + TypeId, + Store, + InitialDragLocation, + PendingDrop, + DimensionState, +} from '../../../src/types'; + +const { home, inHome1 } = getPreset(); + +const origin: Position = { x: 0, y: 0 }; +const noWhere: InitialDragLocation = { + selection: origin, + center: origin, +}; +const noDimensions: DimensionState = { + request: null, + draggable: {}, + droppable: {}, +}; +type LiftFnArgs = { + id: DraggableId, + type: TypeId, + client: InitialDragLocation, + windowScroll: Position, + isScrollAllowed: boolean, +} + +const draggableId: DraggableId = inHome1.id; +const defaultType: TypeId = 'type'; + +const liftDefaults: LiftFnArgs = { + id: draggableId, + type: defaultType, + windowScroll: origin, + client: noWhere, + isScrollAllowed: true, +}; + +const liftWithDefaults = (args?: LiftFnArgs = liftDefaults) => { + const { id, type, client, windowScroll, isScrollAllowed } = args; + return lift(id, type, client, windowScroll, isScrollAllowed); +}; + +const initialState: State = createStore().getState(); + +describe('action creators', () => { + describe('lift', () => { + beforeEach(() => { + jest.useFakeTimers(); + jest.spyOn(console, 'error').mockImplementation(() => { }); + }); + + afterEach(() => { + jest.useRealTimers(); + console.error.mockRestore(); + }); + + // This test is more a baseline for the others + // to ensure that the happy path works correctly + it('should perform a multi phased lift', () => { + const store: Store = createStore(); + jest.spyOn(store, 'dispatch'); + + liftWithDefaults()(store.dispatch, store.getState); + + // Phase 1: flush any existing animations + expect(store.dispatch).toHaveBeenCalledWith(prepare()); + expect(store.dispatch).toHaveBeenCalledTimes(1); + + // Phase 2: request dimensions after flushing animations + jest.runOnlyPendingTimers(); + + expect(store.dispatch).toHaveBeenCalledWith(requestDimensions(defaultType)); + expect(store.dispatch).toHaveBeenCalledTimes(2); + + // publishing some fake dimensions + store.dispatch(publishDroppableDimension(home)); + store.dispatch(publishDraggableDimension(inHome1)); + // now called four times + expect(store.dispatch).toHaveBeenCalledTimes(4); + + // Phase 3: after dimensions are collected complete the lift + jest.runOnlyPendingTimers(); + + expect(store.dispatch).toHaveBeenCalledWith(completeLift( + liftDefaults.id, + liftDefaults.type, + liftDefaults.client, + liftDefaults.windowScroll, + liftDefaults.isScrollAllowed + )); + expect(store.dispatch).toHaveBeenCalledTimes(5); + }); + + describe('flushing previous drop animations', () => { + const dropAnimatingState: State = (() => { + const pending: PendingDrop = { + trigger: 'CANCEL', + newHomeOffset: origin, + impact: noImpact, + result: { + draggableId, + type: defaultType, + source: { + droppableId: 'drop-1', + index: 0, + }, + destination: null, + }, + }; + const state: State = { + phase: 'DROP_ANIMATING', + drag: null, + drop: { + pending, + result: null, + }, + dimension: noDimensions, + }; + return state; + })(); + + it('should flush any existing drop animation', () => { + const dispatch: Function = jest.fn(); + const getState: Function = jest.fn(() => dropAnimatingState); + + liftWithDefaults()(dispatch, getState); + + // $ExpectError - not checking for null in state shape + expect(dispatch).toHaveBeenCalledWith(completeDrop(dropAnimatingState.drop.pending.result)); + expect(dispatch).toHaveBeenCalledWith(prepare()); + expect(console.error).not.toHaveBeenCalled(); + }); + + it('should clean the state and log an error if there is an invalid drop animating state', () => { + const state: State = { + ...initialState, + // hacking the phase + phase: 'DROP_ANIMATING', + }; + const dispatch: Function = jest.fn(); + const getState: Function = jest.fn(() => state); + + liftWithDefaults()(dispatch, getState); + + expect(dispatch).toHaveBeenCalledWith(clean()); + expect(console.error).toHaveBeenCalled(); + }); + + it('should not begin a lift if the drag is cancelled while the animations are flushing', () => { + const store: Store = createStore(); + jest.spyOn(store, 'dispatch'); + + liftWithDefaults()(store.dispatch, store.getState); + // flushing + expect(store.dispatch).toHaveBeenCalledWith(prepare()); + + // need to wait for setTimeout to flush + expect(store.dispatch).toHaveBeenCalledTimes(1); + + // while waiting a cancel occurs + cancel()(store.dispatch, store.getState); + + // because a drag was not occurring the state is cleaned + expect(store.dispatch).toHaveBeenCalledWith(clean()); + // now called two times + expect(store.dispatch).toHaveBeenCalledTimes(2); + + // now ticking the setTimeout + jest.runOnlyPendingTimers(); + + // normally would start requesting dimensions + expect(store.dispatch).not.toHaveBeenCalledWith( + completeLift( + liftDefaults.id, + liftDefaults.type, + liftDefaults.client, + liftDefaults.windowScroll, + liftDefaults.isScrollAllowed + ) + ); + + // dispatch not called since previous clean + expect(store.dispatch).toHaveBeenCalledTimes(2); + }); + }); + + describe('dimensions collected and drag not started', () => { + it('should not continue to lift if cancelled', () => { + const store: Store = createStore(); + jest.spyOn(store, 'dispatch'); + + liftWithDefaults()(store.dispatch, store.getState); + + // Phase 1: flush any existing animations + expect(store.dispatch).toHaveBeenCalledWith(prepare()); + expect(store.dispatch).toHaveBeenCalledTimes(1); + + // Phase 2: request dimensions after flushing animations + jest.runOnlyPendingTimers(); + + expect(store.dispatch).toHaveBeenCalledWith(requestDimensions(defaultType)); + expect(store.dispatch).toHaveBeenCalledTimes(2); + + // drag is now cancelled before all dimensions are published + cancel()(store.dispatch, store.getState); + expect(store.dispatch).toHaveBeenCalledTimes(3); + + // This would usually start phase three: lift + jest.runOnlyPendingTimers(); + + // no increase in the amount of times called + expect(store.dispatch).toHaveBeenCalledTimes(3); + expect(store.dispatch).not.toHaveBeenCalledWith(completeLift( + liftDefaults.id, + liftDefaults.type, + liftDefaults.client, + liftDefaults.windowScroll, + liftDefaults.isScrollAllowed + )); + + // being super careful + jest.runAllTimers(); + expect(store.dispatch).toHaveBeenCalledTimes(3); + + // should be in the idle state + expect(store.getState()).toEqual(initialState); + }); + }); + }); +}); diff --git a/test/utils/dimension.js b/test/utils/dimension.js index dabf7afa1e..91a6797118 100644 --- a/test/utils/dimension.js +++ b/test/utils/dimension.js @@ -2,6 +2,7 @@ import getClientRect from '../../src/state/get-client-rect'; import { getDroppableDimension, getDraggableDimension } from '../../src/state/dimension'; import { add } from '../../src/state/position'; +import { vertical } from '../../src/state/axis'; import type { Axis, Position, @@ -12,7 +13,7 @@ import type { DroppableDimensionMap, } from '../../src/types'; -export const getPreset = (axis: Axis) => { +export const getPreset = (axis?: Axis = vertical) => { const margin: Spacing = { top: 10, left: 10, bottom: 5, right: 5 }; const padding: Spacing = { top: 2, left: 2, bottom: 2, right: 2 }; const windowScroll: Position = { x: 50, y: 100 };