From c3d915d9976423f00de915ac316865c1cedd5e79 Mon Sep 17 00:00:00 2001 From: pradeepnschrodinger Date: Wed, 17 Nov 2021 13:32:19 +0000 Subject: [PATCH] Fix row expansion erroring out. The propsChange reducer was mutating state AND returning a new state object. This is unsupported in immer, and the fix is to only either mutate or only return the new state. I went ahead with mutation as it's much easier on the developer. --- src/reducers/columnStateHelper.js | 3 +- src/reducers/computeRenderedRows.js | 21 +++---- src/reducers/index.js | 93 +++++++++++++---------------- src/reducers/updateRowHeight.js | 3 - 4 files changed, 50 insertions(+), 70 deletions(-) diff --git a/src/reducers/columnStateHelper.js b/src/reducers/columnStateHelper.js index 6e6c58d1..e8bdfad9 100644 --- a/src/reducers/columnStateHelper.js +++ b/src/reducers/columnStateHelper.js @@ -23,7 +23,6 @@ const DRAG_SCROLL_BUFFER = 100; /** * Initialize scrollX state - * TODO (jordan) Audit this method for cases where deep values are not properly cloned * * @param {!Object} state * @param {!Object} props @@ -53,7 +52,7 @@ function initialize(state, props, oldProps) { : isColumnResizing; columnResizingData = isColumnResizing ? columnResizingData : {}; - return Object.assign({}, state, { + Object.assign(state, { columnResizingData, isColumnResizing, maxScrollX, diff --git a/src/reducers/computeRenderedRows.js b/src/reducers/computeRenderedRows.js index e2d988d6..a73cd17d 100644 --- a/src/reducers/computeRenderedRows.js +++ b/src/reducers/computeRenderedRows.js @@ -34,12 +34,11 @@ import updateRowHeight from './updateRowHeight'; * @return {!Object} The updated state object */ export default function computeRenderedRows(state, scrollAnchor) { - const newState = Object.assign({}, state); - let rowRange = calculateRenderedRowRange(newState, scrollAnchor); + let rowRange = calculateRenderedRowRange(state, scrollAnchor); - const { rowSettings, scrollContentHeight } = newState; + const { rowSettings, scrollContentHeight } = state; const { rowsCount } = rowSettings; - const { bodyHeight } = tableHeightsSelector(newState); + const { bodyHeight } = tableHeightsSelector(state); const maxScrollY = scrollContentHeight - bodyHeight; let firstRowOffset; @@ -47,7 +46,7 @@ export default function computeRenderedRows(state, scrollAnchor) { // leave only a subset of rows shown, but no scrollbar to scroll up to the first rows. if (maxScrollY === 0) { if (rowRange.firstViewportIdx > 0) { - rowRange = calculateRenderedRowRange(newState, { + rowRange = calculateRenderedRowRange(state, { firstOffset: 0, lastIndex: rowsCount - 1, }); @@ -61,15 +60,15 @@ export default function computeRenderedRows(state, scrollAnchor) { const firstRowIndex = rowRange.firstViewportIdx; const endRowIndex = rowRange.endViewportIdx; - computeRenderedRowOffsets(newState, rowRange, state.scrolling); + computeRenderedRowOffsets(state, rowRange, state.scrolling); let scrollY = 0; if (rowsCount > 0) { - scrollY = newState.rowOffsets[rowRange.firstViewportIdx] - firstRowOffset; + scrollY = state.rowOffsets[rowRange.firstViewportIdx] - firstRowOffset; } scrollY = clamp(scrollY, 0, maxScrollY); - return Object.assign(newState, { + Object.assign(state, { firstRowIndex, firstRowOffset, endRowIndex, @@ -85,9 +84,6 @@ export default function computeRenderedRows(state, scrollAnchor) { * We use the scrollAnchor to determine what either the first or last row * will be, as well as the offset. * - * NOTE (jordan) This alters state so it shouldn't be called - * without state having been cloned first. - * * @param {!Object} state * @param {{ * firstIndex?: number, @@ -212,9 +208,6 @@ function calculateRenderedRowRange(state, scrollAnchor) { * Walk the rows to render and compute the height offsets and * positions in the row buffer. * - * NOTE (jordan) This alters state so it shouldn't be called - * without state having been cloned first. - * * @param {!Object} state * @param {{ * endBufferIdx: number, diff --git a/src/reducers/index.js b/src/reducers/index.js index cfb58358..3a2d520e 100644 --- a/src/reducers/index.js +++ b/src/reducers/index.js @@ -108,78 +108,70 @@ const slice = createSlice({ initialize(state, action) { const props = action.payload; - let newState = setStateFromProps(state, props); - newState = initializeRowHeightsAndOffsets(newState); - const scrollAnchor = getScrollAnchor(newState, props); - newState = computeRenderedRows(newState, scrollAnchor); - return columnStateHelper.initialize(newState, props, {}); + setStateFromProps(state, props); + initializeRowHeightsAndOffsets(state); + const scrollAnchor = getScrollAnchor(state, props); + computeRenderedRows(state, scrollAnchor); + columnStateHelper.initialize(state, props, {}); }, propChange(state, action) { const { newProps, oldProps } = action.payload; - let newState = setStateFromProps(state, newProps); + const oldState = _.clone(state); + setStateFromProps(state, newProps); if ( oldProps.rowsCount !== newProps.rowsCount || oldProps.rowHeight !== newProps.rowHeight || oldProps.subRowHeight !== newProps.subRowHeight ) { - newState = initializeRowHeightsAndOffsets(newState); + initializeRowHeightsAndOffsets(state); } if (oldProps.rowsCount !== newProps.rowsCount) { - // NOTE (jordan) bad practice to modify state directly, but okay since - // we know setStateFromProps clones state internally - newState.rowBufferSet = new IntegerBufferSet(); + state.rowBufferSet = new IntegerBufferSet(); } - const scrollAnchor = getScrollAnchor(newState, newProps, oldProps); + const scrollAnchor = getScrollAnchor(state, newProps, oldProps); // If anything has changed in state, update our rendered rows - if (!shallowEqual(state, newState) || scrollAnchor.changed) { - newState = computeRenderedRows(newState, scrollAnchor); + if (!shallowEqual(state, oldState) || scrollAnchor.changed) { + computeRenderedRows(state, scrollAnchor); } - newState = columnStateHelper.initialize(newState, newProps, oldProps); + columnStateHelper.initialize(state, newProps, oldProps); // if scroll values have changed, then we're scrolling! if ( - newState.scrollX !== state.scrollX || - newState.scrollY !== state.scrollY + state.scrollX !== oldState.scrollX || + state.scrollY !== oldState.scrollY ) { - newState.scrolling = newState.scrolling || true; + state.scrolling = state.scrolling || true; } // TODO REDUX_MIGRATION solve w/ evil-diff // TODO (jordan) check if relevant props unchanged and // children column widths and flex widths are unchanged // alternatively shallow diff and reconcile props - return newState; }, scrollEnd(state) { - const newState = Object.assign({}, state, { - scrolling: false, - }); + state.scrolling = false; const previousScrollAnchor = { firstIndex: state.firstRowIndex, firstOffset: state.firstRowOffset, lastIndex: state.lastIndex, }; - return computeRenderedRows(newState, previousScrollAnchor); + computeRenderedRows(state, previousScrollAnchor); }, scrollToY(state, action) { - let scrollY = action.payload; - const newState = Object.assign({}, state, { - scrolling: true, - }); - const scrollAnchor = scrollTo(newState, scrollY); - return computeRenderedRows(newState, scrollAnchor); + const scrollY = action.payload; + state.scrolling = true; + const scrollAnchor = scrollTo(state, scrollY); + computeRenderedRows(state, scrollAnchor); }, scrollToX(state, action) { const scrollX = action.payload; - return Object.assign({}, state, { - scrolling: true, - scrollX, - }); + state.scrolling = true; + state.scrollX = scrollX; }, }, }); @@ -202,7 +194,7 @@ function initializeRowHeightsAndOffsets(state) { for (let idx = 0; idx < rowsCount; idx++) { storedHeights[idx] = defaultFullRowHeight; } - return Object.assign({}, state, { + Object.assign(state, { rowOffsetIntervalTree, scrollContentHeight, storedHeights, @@ -223,15 +215,15 @@ function setStateFromProps(state, props) { useGroupHeader, } = convertColumnElementsToData(props.children); - const newState = Object.assign({}, state, { + Object.assign(state, { columnGroupProps, columnProps, elementTemplates, }); - newState.elementHeights = Object.assign( + state.elementHeights = Object.assign( {}, - newState.elementHeights, + state.elementHeights, pick(props, [ 'cellGroupWrapperHeight', 'footerHeight', @@ -240,37 +232,36 @@ function setStateFromProps(state, props) { ]) ); if (!useGroupHeader) { - newState.elementHeights.groupHeaderHeight = 0; + state.elementHeights.groupHeaderHeight = 0; } - newState.rowSettings = Object.assign( + state.rowSettings = Object.assign( {}, - newState.rowSettings, + state.rowSettings, pick(props, ['bufferRowCount', 'rowHeight', 'rowsCount', 'subRowHeight']) ); - const { rowHeight, subRowHeight } = newState.rowSettings; - newState.rowSettings.rowHeightGetter = + const { rowHeight, subRowHeight } = state.rowSettings; + state.rowSettings.rowHeightGetter = props.rowHeightGetter || (() => rowHeight); - newState.rowSettings.subRowHeightGetter = + state.rowSettings.subRowHeightGetter = props.subRowHeightGetter || (() => subRowHeight || 0); - newState.rowSettings.rowAttributesGetter = props.rowAttributesGetter; + state.rowSettings.rowAttributesGetter = props.rowAttributesGetter; - newState.scrollFlags = Object.assign( + state.scrollFlags = Object.assign( {}, - newState.scrollFlags, + state.scrollFlags, pick(props, ['overflowX', 'overflowY', 'showScrollbarX', 'showScrollbarY']) ); - newState.tableSize = Object.assign( + state.tableSize = Object.assign( {}, - newState.tableSize, + state.tableSize, pick(props, ['height', 'maxHeight', 'ownerHeight', 'width']) ); - newState.tableSize.useMaxHeight = newState.tableSize.height === undefined; + state.tableSize.useMaxHeight = state.tableSize.height === undefined; - newState.scrollbarXHeight = props.scrollbarXHeight; - newState.scrollbarYWidth = props.scrollbarYWidth; - return newState; + state.scrollbarXHeight = props.scrollbarXHeight; + state.scrollbarYWidth = props.scrollbarYWidth; } const { reducer, actions } = slice; diff --git a/src/reducers/updateRowHeight.js b/src/reducers/updateRowHeight.js index de5ce896..15fe8e05 100644 --- a/src/reducers/updateRowHeight.js +++ b/src/reducers/updateRowHeight.js @@ -15,9 +15,6 @@ * Update our cached row height for a specific index * based on the value from rowHeightGetter * - * NOTE (jordan) This alters state so it shouldn't be called - * without state having been cloned first. - * * @param {!Object} state * @param {number} rowIdx * @return {number} The new row height