diff --git a/README.md b/README.md index 276b667..4d46c9c 100644 --- a/README.md +++ b/README.md @@ -319,11 +319,13 @@ A developer might prefer to use custom (external) state management rather than t * `getInitialState(): object` — Returns the initial `VirtualScroller` state for the cases when a developer configures `VirtualScroller` for custom (external) state management. -* `useState({ getState, updateState })` — Enables custom (external) state management. +* `useState({ getState, setState, updateState? })` — Enables custom (external) state management. * `getState(): object` — Returns the externally managed `VirtualScroller` `state`. - * `updateState(stateUpdate: object)` — Updates the externally managed `VirtualScroller` `state`. Must call `.onRender()` right after the updated `state` gets "rendered". A higher-order `VirtualScroller` implementation could either "render" the list immediately in its `updateState()` function, like the DOM implementation does, or the `updateState()` function could "schedule" a "re-render", like the React implementation does, in which case such `updateState()` function would be called an ["asynchronous"](https://reactjs.org/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous) one, meaning that state updates aren't "rendered" immediately and are instead queued and then "rendered" in a single compound state update for better performance. + * `setState(newState: object)` — Sets the externally managed `VirtualScroller` `state`. Must call `.onRender()` right after the updated `state` gets "rendered". A higher-order `VirtualScroller` implementation could either "render" the list immediately in its `setState()` function, in which case it would be better to use the default state management instead and pass a custom `render()` function, or the `setState()` function could "schedule" an "asynchronous" "re-render", like the React implementation does, in which case such `setState()` function would be called an ["asynchronous"](https://reactjs.org/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous) one, meaning that state updates aren't "rendered" immediately and are instead queued and then "rendered" in a single compound state update for better performance. + + * `updateState(stateUpdate: object)` — (optional) `setState()` parameter could be replaced with `updateState()` parameter. The only difference between the two is that `updateState()` gets called with just the portion of the state that is being updated while `setState()` gets called with the whole updated state object, so it's just a matter of preference. For a usage example, see `./source/react/VirtualScroller.js`. The steps are: @@ -343,7 +345,7 @@ When using custom (external) state management, contrary to the default (internal #### "Advanced" (rarely used) instance methods -* `onItemHeightDidChange(i: number)` — (advanced) If an item's height could've changed, this function should be called immediately after the item's height has potentially changed. An example would be having an "Expand"/"Collapse" button in a list item. The function re-measures the item's height and re-calculates `VirtualScroller` layout. +* `onItemHeightDidChange(i: number)` — (advanced) If an item's height could've changed, this function should be called immediately after the item's height has potentially changed. The function re-measures the item's height (the item must still be rendered) and re-calculates `VirtualScroller` layout. An example for using this function would be having an "Expand"/"Collapse" button in a list item. * There's also a convention that every change in an item's height must come as a result of changing its "state", be it the item's state that is stored internally in the `VirtualScroller` state (see `.setItemState()` function) or some other application-level state that is stored outside of the `VirtualScroller` state. @@ -467,7 +469,7 @@ The required properties are: * `item: any` — The item object itself (an element of the `items` array). * `state: any?` — Item's state. See the description of `itemStates` property of `VirtualScroller` `state` for more details. * `setState(newState: any?)` — Can be called to replace item's state. See the description of `setItemState(i, newState)` function of `VirtualScroller` for more details. - * `onHeightDidChange(i)` — If an item's height could change after the initial render, this function should be called immediately after the item's height has potentially changed. See the description of `onItemHeightDidChange()` function of `VirtualScroller` for more details. + * `onHeightDidChange(i)` — If an item's height could change after the initial render, this function should be called immediately after the item's height has potentially changed — in a `useLayoutEffect()`. See the description of `onItemHeightDidChange()` function of `VirtualScroller` for more details. * For best performance, make sure that `itemComponent` is a `React.memo()` component or a `React.PureComponent`. Otherwise, list items will keep re-rendering themselves as the user scrolls because the containing `` component gets re-rendered on scroll. diff --git a/index.cjs b/index.cjs index 2e6c7df..6d21195 100644 --- a/index.cjs +++ b/index.cjs @@ -1,4 +1,6 @@ 'use strict' exports = module.exports = require('./commonjs/VirtualScroller.js').default + +exports.ItemNotRenderedError = require('./commonjs/ItemNotRenderedError.js').default exports['default'] = require('./commonjs/VirtualScroller.js').default \ No newline at end of file diff --git a/index.d.ts b/index.d.ts index ba631de..648a5c7 100644 --- a/index.d.ts +++ b/index.d.ts @@ -99,3 +99,9 @@ export default class VirtualScroller { getInitialState(): State; useState(options: UseStateOptions): void; } + +export class ItemNotRenderedError { + constructor( + message: string + ); +} \ No newline at end of file diff --git a/index.js b/index.js index bb5e9ec..5bc7c9f 100644 --- a/index.js +++ b/index.js @@ -1 +1,2 @@ export { default } from './modules/VirtualScroller.js' +export { default as ItemNotRenderedError } from './modules/ItemNotRenderedError.js' diff --git a/package-lock.json b/package-lock.json index 6179ca7..d651597 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "virtual-scroller", - "version": "1.11.3", + "version": "1.12.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 6050298..5180450 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "virtual-scroller", - "version": "1.11.3", + "version": "1.12.0", "description": "A component for efficiently rendering large lists of variable height items", "main": "index.cjs", "module": "index.js", diff --git a/source/DOM/ItemsContainer.js b/source/DOM/ItemsContainer.js index 242cc6d..e241f41 100644 --- a/source/DOM/ItemsContainer.js +++ b/source/DOM/ItemsContainer.js @@ -1,3 +1,5 @@ +import ItemNotRenderedError from '../ItemNotRenderedError.js' + export default class ItemsContainer { /** * Constructs a new "container" from an element. @@ -10,9 +12,12 @@ export default class ItemsContainer { _getNthRenderedItemElement(renderedElementIndex) { const childNodes = this.getElement().childNodes if (renderedElementIndex > childNodes.length - 1) { - console.log('~ Items Container Contents ~') - console.log(this.getElement().innerHTML) - throw new Error(`Element with index ${renderedElementIndex} was not found in the list of Rendered Item Elements in the Items Container of Virtual Scroller. There're only ${childNodes.length} Elements there.`) + // console.log('~ Items Container Contents ~') + // console.log(this.getElement().innerHTML) + throw new ItemNotRenderedError({ + renderedElementIndex, + renderedElementsCount: childNodes.length + }) } return childNodes[renderedElementIndex] } diff --git a/source/ItemNotRenderedError.js b/source/ItemNotRenderedError.js new file mode 100644 index 0000000..56d1741 --- /dev/null +++ b/source/ItemNotRenderedError.js @@ -0,0 +1,16 @@ +export default class ItemNotRenderedError extends Error { + constructor({ + renderedElementIndex, + renderedElementsCount, + message + }) { + super(message || getDefaultMessage({ renderedElementIndex, renderedElementsCount })) + } +} + +function getDefaultMessage({ + renderedElementIndex, + renderedElementsCount +}) { + return `Element with index ${renderedElementIndex} was not found in the list of Rendered Item Elements in the Items Container of Virtual Scroller. There're only ${renderedElementsCount} Elements there.` +} \ No newline at end of file diff --git a/source/Layout.test.js b/source/Layout.test.js index f3cef2e..4bce4a4 100644 --- a/source/Layout.test.js +++ b/source/Layout.test.js @@ -148,6 +148,10 @@ describe('Layout', function() { let shouldResetGridLayout + const errors = [] + + global.VirtualScrollerCatchError = (error) => errors.push(error) + layout.getLayoutUpdateForItemsDiff( { firstShownItemIndex: 3, @@ -171,6 +175,11 @@ describe('Layout', function() { afterItemsHeight: 5 * (ITEM_HEIGHT + VERTICAL_SPACING) }) + global.VirtualScrollerCatchError = undefined + errors.length.should.equal(2) + errors[0].message.should.equal('[virtual-scroller] ~ Prepended items count 5 is not divisible by Columns Count 4 ~') + errors[1].message.should.equal('[virtual-scroller] Layout reset required') + shouldResetGridLayout.should.equal(true) }) }) \ No newline at end of file diff --git a/source/VirtualScroller.js b/source/VirtualScroller.js index 2b26bf7..3ee46ca 100644 --- a/source/VirtualScroller.js +++ b/source/VirtualScroller.js @@ -35,6 +35,8 @@ export default class VirtualScroller { const isRestart = this._isActive === false if (!isRestart) { + this.waitingForRender = true + // If no custom state storage has been configured, use the default one. // Also sets the initial state. if (!this._usesCustomStateStorage) { diff --git a/source/VirtualScroller.layout.js b/source/VirtualScroller.layout.js index 4bfd30a..13e74de 100644 --- a/source/VirtualScroller.layout.js +++ b/source/VirtualScroller.layout.js @@ -7,6 +7,8 @@ import { setTimeout, clearTimeout } from 'request-animation-frame-timeout' import log, { warn, isDebug, reportError } from './utility/debug.js' import { LAYOUT_REASON } from './Layout.js' +import ItemNotRenderedError from './ItemNotRenderedError.js' + export default function() { this.onUpdateShownItemIndexes = ({ reason, stateUpdate }) => { // In case of "don't do anything". @@ -149,6 +151,9 @@ export default function() { // Set `this.firstNonMeasuredItemIndex`. this.firstNonMeasuredItemIndex = firstNonMeasuredItemIndex + // if (firstNonMeasuredItemIndex !== undefined) { + // log('Non-measured item index that will be measured at next layout', firstNonMeasuredItemIndex) + // } // Set "previously calculated layout". // @@ -340,20 +345,21 @@ export default function() { // rather than from scratch, which would be an optimization. // function updatePreviouslyCalculatedLayoutOnItemHeightChange(i, previousHeight, newHeight) { - if (this.previouslyCalculatedLayout) { + const prevLayout = this.previouslyCalculatedLayout + if (prevLayout) { const heightDifference = newHeight - previousHeight - if (i < this.previouslyCalculatedLayout.firstShownItemIndex) { - // Patch `this.previouslyCalculatedLayout`'s `.beforeItemsHeight`. - this.previouslyCalculatedLayout.beforeItemsHeight += heightDifference - } else if (i > this.previouslyCalculatedLayout.lastShownItemIndex) { - // Could patch `.afterItemsHeight` of `this.previouslyCalculatedLayout` here, - // if `.afterItemsHeight` property existed in `this.previouslyCalculatedLayout`. - if (this.previouslyCalculatedLayout.afterItemsHeight !== undefined) { - this.previouslyCalculatedLayout.afterItemsHeight += heightDifference + if (i < prevLayout.firstShownItemIndex) { + // Patch `prevLayout`'s `.beforeItemsHeight`. + prevLayout.beforeItemsHeight += heightDifference + } else if (i > prevLayout.lastShownItemIndex) { + // Could patch `.afterItemsHeight` of `prevLayout` here, + // if `.afterItemsHeight` property existed in `prevLayout`. + if (prevLayout.afterItemsHeight !== undefined) { + prevLayout.afterItemsHeight += heightDifference } } else { - // Patch `this.previouslyCalculatedLayout`'s shown items height. - this.previouslyCalculatedLayout.shownItemsHeight += newHeight - previousHeight + // Patch `prevLayout`'s shown items height. + prevLayout.shownItemsHeight += newHeight - previousHeight } } } @@ -371,7 +377,7 @@ export default function() { } this._onItemHeightDidChange = (i) => { - log('~ Re-measure item height ~') + log('~ On Item Height Did Change was called ~') log('Item index', i) const { @@ -412,24 +418,57 @@ export default function() { const previousHeight = itemHeights[i] if (previousHeight === undefined) { - return reportError(`"onItemHeightDidChange()" has been called for item ${i}, but that item hasn't been rendered before.`) + return reportError(`"onItemHeightDidChange()" has been called for item index ${i} but the item hasn't been rendered before.`) } - const newHeight = remeasureItemHeight.call(this, i) + log('~ Re-measure item height ~') + + let newHeight + + try { + newHeight = remeasureItemHeight.call(this, i) + } catch (error) { + // Successfully finishing an `onItemHeightDidChange(i)` call is not considered + // critical for `VirtualScroller`'s operation, so such errors could be ignored. + if (error instanceof ItemNotRenderedError) { + return reportError(`"onItemHeightDidChange()" has been called for item index ${i} but the item is not currently rendered and can\'t be measured. The exact error was: ${error.message}`) + } + } log('Previous height', previousHeight) log('New height', newHeight) if (previousHeight !== newHeight) { - log('~ Item height has changed ~') + log('~ Item height has changed. Should update layout. ~') - // Update or reset previously calculated layout. + // Update or reset a previously calculated layout + // so that the "diff"s based on that layout in the future + // produce correct results. updatePreviouslyCalculatedLayoutOnItemHeightChange.call(this, i, previousHeight, newHeight) // Recalculate layout. - this.onUpdateShownItemIndexes({ reason: LAYOUT_REASON.ITEM_HEIGHT_CHANGED }) + // + // If the `VirtualScroller` is already waiting for a state update to be rendered, + // delay `onItemHeightDidChange(i)`'s re-layout until that state update is rendered. + // The reason is that React ``'s `onHeightDidChange()` is meant to + // be called inside `useLayoutEffect()` hook. Due to how React is implemented internally, + // that might happen in the middle of the currently pending `setState()` operation + // being applied, resulting in weird "race condition" bugs. + // + if (this.waitingForRender) { + log('~ Another state update is already waiting to be rendered. Delay the layout update until then. ~') + this.updateLayoutAfterRenderBecauseItemHeightChanged = true + } else { + this.onUpdateShownItemIndexes({ reason: LAYOUT_REASON.ITEM_HEIGHT_CHANGED }) + } - // Schedule the item height update for after the new items have been rendered. + // If there was a request for `setState()` with new `items`, then the changes + // to `currentState.itemHeights[]` made above in a `remeasureItemHeight()` call + // would be overwritten when that pending `setState()` call gets applied. + // To fix that, the updates to current `itemHeights[]` are noted in + // `this.itemHeightsThatChangedWhileNewItemsWereBeingRendered` variable. + // That variable is then checked when the `setState()` call with the new `items` + // has been updated. if (this.newItemsWillBeRendered) { if (!this.itemHeightsThatChangedWhileNewItemsWereBeingRendered) { this.itemHeightsThatChangedWhileNewItemsWereBeingRendered = {} diff --git a/source/VirtualScroller.onRender.js b/source/VirtualScroller.onRender.js index b5862b3..d6dbac2 100644 --- a/source/VirtualScroller.onRender.js +++ b/source/VirtualScroller.onRender.js @@ -1,4 +1,4 @@ -import log, { warn, isDebug } from './utility/debug.js' +import log, { warn, reportError, isDebug } from './utility/debug.js' import getStateSnapshot from './utility/getStateSnapshot.js' import shallowEqual from './utility/shallowEqual.js' import { LAYOUT_REASON } from './Layout.js' @@ -11,6 +11,8 @@ export default function() { * @param {object} [prevState] */ this._onRender = (newState, prevState) => { + this.waitingForRender = false + log('~ Rendered ~') if (isDebug()) { log('State', getStateSnapshot(newState)) @@ -33,19 +35,58 @@ export default function() { ) } - if (!prevState) { - return + // `this.mostRecentlySetState` checks that state management behavior is correct: + // that in situations when there're multiple new states waiting to be set, + // only the latest one gets applied. + // It keeps the code simpler and prevents possible race condition bugs. + // For example, `VirtualScroller` keeps track of its latest requested + // state update in different instance variable flags which assume that + // only that latest requested state update gets actually applied. + // + // This check should also be performed for the initial render in order to + // guarantee that no potentially incorrect state update goes unnoticed. + // Incorrect state updates could happen when `VirtualScroller` state + // is managed externally by passing `getState()`/`updateState()` options. + // + // Perform the check only when `this.mostRecentSetStateValue` is defined. + // `this.mostRecentSetStateValue` is normally gonna be `undefined` at the initial render + // because the initial state is not set by calling `this.updateState()`. + // At the same time, it is possible that the initial render is delayed + // for whatever reason, and `this.updateState()` gets called before the initial render, + // so `this.mostRecentSetStateValue` could also be defined at the initial render, + // in which case the check should be performed. + // + if (this.mostRecentSetStateValue) { + // "Shallow equality" is used here instead of "strict equality" + // because a developer might choose to supply an `updateState()` function + // rather than a `setState()` function, in which case the `updateState()` function + // would construct its own state object. + if (!shallowEqual(newState, this.mostRecentSetStateValue)) { + warn('The most recent state that was set', getStateSnapshot(this.mostRecentSetStateValue)) + reportError('The state that has been rendered is not the most recent one that was set') + } } // `this.resetStateUpdateFlags()` must be called before calling // `this.measureItemHeightsAndSpacing()`. const { nonMeasuredItemsHaveBeenRendered, + itemHeightHasChanged, widthHasChanged } = resetStateUpdateFlags.call(this) let layoutUpdateReason + if (this.updateLayoutAfterRenderBecauseItemHeightChanged) { + layoutUpdateReason = LAYOUT_REASON.ITEM_HEIGHT_CHANGED + } + + if (!prevState) { + if (!layoutUpdateReason) { + return + } + } + // If the `VirtualScroller`, while calculating layout parameters, encounters // a not-shown item with a non-measured height, it calls `updateState()` just to // render that item first, and then, after the list has been re-rendered, it measures @@ -74,41 +115,43 @@ export default function() { this.verticalSpacing = undefined } - const { items: previousItems } = prevState - const { items: newItems } = newState - // Even if `this.newItemsWillBeRendered` flag is `true`, - // `newItems` could still be equal to `previousItems`. - // For example, when `updateState()` calls don't update `state` immediately - // and a developer first calls `setItems(newItems)` and then calls `setItems(oldItems)`: - // in that case, `this.newItemsWillBeRendered` flag will be `true` but the actual `items` - // in state wouldn't have changed due to the first `updateState()` call being overwritten - // by the second `updateState()` call (that's called "batching state updates" in React). - if (newItems !== previousItems) { - const itemsDiff = this.getItemsDiff(previousItems, newItems) - if (itemsDiff) { - // The call to `.onPrepend()` must precede the call to `.measureItemHeights()` - // which is called in `.onRender()`. - // `this.itemHeights.onPrepend()` updates `firstMeasuredItemIndex` - // and `lastMeasuredItemIndex` of `this.itemHeights`. - const { prependedItemsCount } = itemsDiff - this.itemHeights.onPrepend(prependedItemsCount) - } else { - this.itemHeights.reset() - } + if (prevState) { + const { items: previousItems } = prevState + const { items: newItems } = newState + // Even if `this.newItemsWillBeRendered` flag is `true`, + // `newItems` could still be equal to `previousItems`. + // For example, when `updateState()` calls don't update `state` immediately + // and a developer first calls `setItems(newItems)` and then calls `setItems(oldItems)`: + // in that case, `this.newItemsWillBeRendered` flag will be `true` but the actual `items` + // in state wouldn't have changed due to the first `updateState()` call being overwritten + // by the second `updateState()` call (that's called "batching state updates" in React). + if (newItems !== previousItems) { + const itemsDiff = this.getItemsDiff(previousItems, newItems) + if (itemsDiff) { + // The call to `.onPrepend()` must precede the call to `.measureItemHeights()` + // which is called in `.onRender()`. + // `this.itemHeights.onPrepend()` updates `firstMeasuredItemIndex` + // and `lastMeasuredItemIndex` of `this.itemHeights`. + const { prependedItemsCount } = itemsDiff + this.itemHeights.onPrepend(prependedItemsCount) + } else { + this.itemHeights.reset() + } - if (!widthHasChanged) { - // The call to `this.onNewItemsRendered()` must precede the call to - // `.measureItemHeights()` which is called in `.onRender()` because - // `this.onNewItemsRendered()` updates `firstMeasuredItemIndex` and - // `lastMeasuredItemIndex` of `this.itemHeights` in case of a prepend. - // - // If after prepending items the scroll position - // should be "restored" so that there's no "jump" of content - // then it means that all previous items have just been rendered - // in a single pass, and there's no need to update layout again. - // - if (onNewItemsRendered.call(this, itemsDiff, newState) !== 'SEAMLESS_PREPEND') { - layoutUpdateReason = LAYOUT_REASON.ITEMS_CHANGED + if (!widthHasChanged) { + // The call to `this.onNewItemsRendered()` must precede the call to + // `.measureItemHeights()` which is called in `.onRender()` because + // `this.onNewItemsRendered()` updates `firstMeasuredItemIndex` and + // `lastMeasuredItemIndex` of `this.itemHeights` in case of a prepend. + // + // If after prepending items the scroll position + // should be "restored" so that there's no "jump" of content + // then it means that all previous items have just been rendered + // in a single pass, and there's no need to update layout again. + // + if (onNewItemsRendered.call(this, itemsDiff, newState) !== 'SEAMLESS_PREPEND') { + layoutUpdateReason = LAYOUT_REASON.ITEMS_CHANGED + } } } } @@ -124,9 +167,11 @@ export default function() { // item height measurements is required. // if ( - newState.firstShownItemIndex !== prevState.firstShownItemIndex || - newState.lastShownItemIndex !== prevState.lastShownItemIndex || - newState.items !== prevState.items || + (prevState && ( + newState.firstShownItemIndex !== prevState.firstShownItemIndex || + newState.lastShownItemIndex !== prevState.lastShownItemIndex || + newState.items !== prevState.items + )) || widthHasChanged ) { const verticalSpacingStateUpdate = this.measureItemHeightsAndSpacing() @@ -202,14 +247,14 @@ export default function() { // See if any items' heights changed while new items were being rendered. if (this.itemHeightsThatChangedWhileNewItemsWereBeingRendered) { for (const i of Object.keys(this.itemHeightsThatChangedWhileNewItemsWereBeingRendered)) { - itemHeights[prependedItemsCount + parseInt(i)] = this.itemHeightsThatChangedWhileNewItemsWereBeingRendered[i] + itemHeights[prependedItemsCount + Number(i)] = this.itemHeightsThatChangedWhileNewItemsWereBeingRendered[i] } } // See if any items' states changed while new items were being rendered. if (this.itemStatesThatChangedWhileNewItemsWereBeingRendered) { for (const i of Object.keys(this.itemStatesThatChangedWhileNewItemsWereBeingRendered)) { - itemStates[prependedItemsCount + parseInt(i)] = this.itemStatesThatChangedWhileNewItemsWereBeingRendered[i] + itemStates[prependedItemsCount + Number(i)] = this.itemStatesThatChangedWhileNewItemsWereBeingRendered[i] } } @@ -325,6 +370,9 @@ export default function() { // Read `this.firstNonMeasuredItemIndex` flag. const nonMeasuredItemsHaveBeenRendered = this.firstNonMeasuredItemIndex !== undefined + if (nonMeasuredItemsHaveBeenRendered) { + log('Non-measured item index', this.firstNonMeasuredItemIndex) + } // Reset `this.firstNonMeasuredItemIndex` flag. this.firstNonMeasuredItemIndex = undefined @@ -337,8 +385,13 @@ export default function() { // Reset `this.itemStatesThatChangedWhileNewItemsWereBeingRendered`. this.itemStatesThatChangedWhileNewItemsWereBeingRendered = undefined + // Reset `this.updateLayoutAfterRenderBecauseItemHeightChanged`. + const itemHeightHasChanged = this.updateLayoutAfterRenderBecauseItemHeightChanged + this.updateLayoutAfterRenderBecauseItemHeightChanged = undefined + return { nonMeasuredItemsHaveBeenRendered, + itemHeightHasChanged, widthHasChanged } } diff --git a/source/VirtualScroller.state.js b/source/VirtualScroller.state.js index 6fb2c69..dff437b 100644 --- a/source/VirtualScroller.state.js +++ b/source/VirtualScroller.state.js @@ -52,7 +52,13 @@ export default function createStateHelpers({ this.getState().itemStates[i] = newItemState - // Schedule the item state update for after the new items have been rendered. + // If there was a request for `setState()` with new `items`, then the changes + // to `currentState.itemStates[]` made above would be overwritten when that + // pending `setState()` call gets applied. + // To fix that, the updates to current `itemStates[]` are noted in + // `this.itemStatesThatChangedWhileNewItemsWereBeingRendered` variable. + // That variable is then checked when the `setState()` call with the new `items` + // has been updated. if (this.newItemsWillBeRendered) { if (!this.itemStatesThatChangedWhileNewItemsWereBeingRendered) { this.itemStatesThatChangedWhileNewItemsWereBeingRendered = {} @@ -78,9 +84,25 @@ export default function createStateHelpers({ } this._isSettingNewItems = undefined - // Update `state`. + this.waitingForRender = true + + // Store previous `state`. this.previousState = this.getState() - this._updateState(stateUpdate) + + // If it's the first call to `this.updateState()` then initialize + // the most recent `setState()` value to be the current state. + if (!this.mostRecentSetStateValue) { + this.mostRecentSetStateValue = this.getState() + } + + // Accumulates all "pending" state updates until they have been applied. + this.mostRecentSetStateValue = { + ...this.mostRecentSetStateValue, + ...stateUpdate + } + + // Update `state`. + this._setState(this.mostRecentSetStateValue, stateUpdate) } this.getInitialState = () => { @@ -92,6 +114,7 @@ export default function createStateHelpers({ this.useState = ({ getState, + setState, updateState }) => { if (this._isActive) { @@ -103,17 +126,28 @@ export default function createStateHelpers({ } if (render) { - throw new Error('[virtual-scroller] Creating a `VirtualScroller` class instance with a `render()` parameter means using the default (internal) state storage') + throw new Error('[virtual-scroller] Creating a `VirtualScroller` class instance with a `render()` parameter implies using the default (internal) state storage') } - if (!getState || !updateState) { - throw new Error('[virtual-scroller] When using a custom state storage, one must supply both `getState()` and `updateState()` functions') + if (setState && updateState) { + throw new Error('[virtual-scroller] When using a custom state storage, one must supply either `setState()` or `updateState()` function but not both') + } + + if (!getState || !(setState || updateState)) { + throw new Error('[virtual-scroller] When using a custom state storage, one must supply both `getState()` and `setState()`/`updateState()` functions') } this._usesCustomStateStorage = true this._getState = getState - this._updateState = updateState + + this._setState = (newState, stateUpdate) => { + if (setState) { + setState(newState) + } else { + updateState(stateUpdate) + } + } } this.useDefaultStateStorage = () => { @@ -121,9 +155,9 @@ export default function createStateHelpers({ throw new Error('[virtual-scroller] When using the default (internal) state management, one must supply a `render(state, prevState)` function parameter') } - // Create default `getState()`/`updateState()` functions. + // Create default `getState()`/`setState()` functions. this._getState = defaultGetState.bind(this) - this._updateState = defaultUpdateState.bind(this) + this._setState = defaultSetState.bind(this) // When `state` is stored externally, a developer is responsible for // initializing it with the initial value. @@ -140,17 +174,20 @@ export default function createStateHelpers({ this.state = newState } - function defaultUpdateState(stateUpdate) { - // Because this variant of `.updateState()` is "synchronous" (immediate), - // it can be written like `...prevState`, and no state updates would be lost. - // But if it was "asynchronous" (not immediate), then `...prevState` - // wouldn't work in all cases, because it could be stale in cases - // when more than a single `updateState()` call is made before - // the state actually updates, making `prevState` stale. - this.state = { - ...this.state, - ...stateUpdate - } + function defaultSetState(newState, stateUpdate) { + // // Because the default state updates are "synchronous" (immediate), + // // the `...stateUpdate` could be applied over `...this.state`, + // // and no state updates would be lost. + // // But if it was "asynchronous" (not immediate), then `...this.state` + // // wouldn't work in all cases, because it could be stale in cases + // // when more than a single `setState()` call is made before + // // the state actually updates, making some properties of `this.state` stale. + // this.state = { + // ...this.state, + // ...stateUpdate + // } + + this.state = newState render(this.state, this.previousState) diff --git a/source/react/VirtualScroller.js b/source/react/VirtualScroller.js index 459421e..c753353 100644 --- a/source/react/VirtualScroller.js +++ b/source/react/VirtualScroller.js @@ -8,32 +8,21 @@ import useInstanceMethods from './useInstanceMethods.js' import useItemKeys from './useItemKeys.js' import useSetItemState from './useSetItemState.js' import useOnItemHeightDidChange from './useOnItemHeightDidChange.js' -import useHandleItemsPropertyChange from './useHandleItemsPropertyChange.js' -import useHandleItemIndexesChange from './useHandleItemIndexesChange.js' +import useSetNewItemsOnItemsPropertyChange from './useSetNewItemsOnItemsPropertyChange.js' +import useUpdateItemKeysOnItemsChange from './useUpdateItemKeysOnItemsChange.js' import useClassName from './useClassName.js' import useStyle from './useStyle.js' -// When `items` property changes, `useHandleItemsPropertyChange()` hook detects that -// and calls `VirtualScroller.setItems()` which in turn calls the `updateState()` function. -// At this point, an insignificant optimization could be applied: -// the component could avoid re-rendering the second time. -// Instead, the state update could be applied "immediately" if it originated -// from `.setItems()` function call, eliminating the unneeded second re-render. -// -// I could see how this minor optimization could get brittle when modifiying the code, -// so I put it under a feature flag so that it could potentially be turned off -// in case of any potential weird issues in some future. -// -// Another reason for using this feature is: -// -// Since `useHandleItemsPropertyChange()` runs at render time -// and not after the render has finished (not in an "effect"), -// if the state update was done "conventionally" (by calling `_setNewState()`), -// React would throw an error about updating state during render. -// No one knows what the original error message was. -// Perhaps it's no longer relevant in newer versions of React. -// -const USE_ITEMS_UPDATE_NO_SECOND_RENDER_OPTIMIZATION = true +// When `items` property changes: +// * A new `items` property is supplied to the React component. +// * The React component re-renders itself. +// * `useSetNewItemsOnItemsPropertyChange()` hook is run. +// * `useSetNewItemsOnItemsPropertyChange()` hook detects that the `items` property +// has changed and calls `VirtualScroller.setItems(items)`. +// * `VirtualScroller.setItems(items)` calls `VirtualScroller.setState()`. +// * `VirtualScroller.setState()` calls the `setState()` function. +// * The `setState()` function calls a setter from a `useState()` hook. +// * The React component re-renders itself the second time. function VirtualScroller({ as: AsComponent, @@ -113,20 +102,19 @@ function VirtualScroller({ // This way, React will re-render the component on every state update. const { getState, - updateState, - getNextState + setState, + stateToRender } = useState({ initialState: _initialState, onRender: virtualScroller.onRender, - itemsProperty, - USE_ITEMS_UPDATE_NO_SECOND_RENDER_OPTIMIZATION + itemsProperty }) // Use custom (external) state storage in the `VirtualScroller`. useMemo(() => { virtualScroller.useState({ getState, - updateState + setState }) }, []) @@ -138,6 +126,7 @@ function VirtualScroller({ // "reuse" `itemComponent`s in cases when `items` are changed. const { getItemKey, + usesAutogeneratedItemKeys, updateItemKeysForNewItems } = useItemKeys({ getItemId @@ -158,19 +147,18 @@ function VirtualScroller({ }) // Calls `.setItems()` if `items` property has changed. - useHandleItemsPropertyChange(itemsProperty, { + useSetNewItemsOnItemsPropertyChange(itemsProperty, { virtualScroller, // `preserveScrollPosition` property name is deprecated, // use `preserveScrollPositionOnPrependItems` property instead. preserveScrollPosition, - preserveScrollPositionOnPrependItems, - nextItems: getNextState().items + preserveScrollPositionOnPrependItems }) // Updates `key`s if item indexes have changed. - useHandleItemIndexesChange({ + useUpdateItemKeysOnItemsChange(stateToRender.items, { virtualScroller, - itemsBeingRendered: getNextState().items, + usesAutogeneratedItemKeys, updateItemKeysForNewItems }) @@ -209,7 +197,7 @@ function VirtualScroller({ const style = useStyle({ tbody, - getNextState + state: stateToRender }) const { @@ -217,7 +205,7 @@ function VirtualScroller({ itemStates, firstShownItemIndex, lastShownItemIndex - } = getNextState() + } = stateToRender return ( { - if (!getItemId) { + if (usesAutogeneratedItemKeys) { generateItemKeyPrefix() + log('React: ~ Item key prefix:', itemKeyPrefix.current) } }, [ - getItemId, + usesAutogeneratedItemKeys, generateItemKeyPrefix ]) @@ -54,6 +60,7 @@ export default function useItemKeys({ getItemId }) { return { getItemKey, + usesAutogeneratedItemKeys, updateItemKeysForNewItems: generateItemKeyPrefixIfNotUsingItemIds } } \ No newline at end of file diff --git a/source/react/useOnChange.js b/source/react/useOnChange.js new file mode 100644 index 0000000..5f6a1a9 --- /dev/null +++ b/source/react/useOnChange.js @@ -0,0 +1,11 @@ +import { useRef } from 'react' + +export default function useOnChange(value, onChange) { + const previousValueRef = useRef(value) + const previousValue = previousValueRef.current + previousValueRef.current = value + + if (value !== previousValue) { + onChange(value, previousValue) + } +} \ No newline at end of file diff --git a/source/react/useSetNewItemsOnItemsPropertyChange.js b/source/react/useSetNewItemsOnItemsPropertyChange.js new file mode 100644 index 0000000..ee4d395 --- /dev/null +++ b/source/react/useSetNewItemsOnItemsPropertyChange.js @@ -0,0 +1,97 @@ +import log from '../utility/debug.js' + +import useOnChange from './useOnChange.js' + +// If new `items` property is passed: +// +// * Store the scroll Y position for the first one of the current items +// so that it could potentially (in some cases) be restored after the +// new `items` are rendered. +// +// * Call `VirtualScroller.setItems()` function. +// +export default function useSetNewItemsOnItemsPropertyChange(itemsProperty, { + virtualScroller, + // `preserveScrollPosition` property name is deprecated, + // use `preserveScrollPositionOnPrependItems` property instead. + preserveScrollPosition, + preserveScrollPositionOnPrependItems +}) { + // During render, check if the `items` list has changed. + // If it has, capture the Y scroll position and updated item element `key`s. + + // A long "advanced" sidenote on why capturing scroll Y position + // is done during render instead of in an "effect": + // + // Previously, capturing scroll Y position was being done in `useLayoutEffect()` + // but it was later found out that it wouldn't work for a "Show previous" button + // scenario because that button would get hidden by the time `useLayoutEffect()` + // gets called when there're no more "previous" items to show. + // + // Consider this code example: + // + // const { fromIndex, items } = this.state + // const items = allItems.slice(fromIndex) + // return ( + // {fromIndex > 0 && + // + // } + // + // ) + // + // Consider a user clicks "Show previous" to show the items from the start. + // By the time `componentDidUpdate()` is called on ``, + // the "Show previous" button has already been hidden + // (because there're no more "previous" items) + // which results in the scroll Y position jumping forward + // by the height of that "Show previous" button. + // This is because `` captures scroll Y + // position when items are prepended via `.setItems()` + // when the "Show previous" button is still being shown, + // and then restores scroll Y position in `.onRender()` + // when the "Show previous" button has already been hidden: + // that's the reason for the scroll Y "jump". + // + // To prevent that, scroll Y position is captured at `render()` + // time rather than later in `componentDidUpdate()`: this way, + // scroll Y position is captured while the "Show previous" button + // is still being shown. + + useOnChange(itemsProperty, (itemsProperty, prevItemsProperty) => { + log('React: ~ Different `items` property has been passed', itemsProperty) + + let shouldUpdateItems = true + + // Analyze the upcoming `items` change. + const itemsDiff = virtualScroller.getItemsDiff(prevItemsProperty, itemsProperty) + + // `itemsDiff` will be `undefined` in case of a non-incremental items list change. + if (itemsDiff) { + const { + prependedItemsCount, + appendedItemsCount + } = itemsDiff + if (prependedItemsCount === 0 && appendedItemsCount === 0) { + // The items order hasn't changed. + // No need to update them in `VirtualScroller` or to snapshot the Y scroll position. + log('React: ~ The `items` elements are identical to the previous ones') + shouldUpdateItems = false + } + } + + if (shouldUpdateItems) { + // Make a request to update the `items` in `VirtualScroller`. + // This will result in a `setState()` call. + // The new items won't be rendered until that state update is applied. + virtualScroller.setItems(itemsProperty, { + // `preserveScrollPosition` property name is deprecated, + // use `preserveScrollPositionOnPrependItems` property instead. + preserveScrollPositionOnPrependItems: preserveScrollPositionOnPrependItems || preserveScrollPosition + }) + } + }) +} \ No newline at end of file diff --git a/source/react/useState.js b/source/react/useState.js index 1333d5c..873df61 100644 --- a/source/react/useState.js +++ b/source/react/useState.js @@ -1,99 +1,187 @@ -import { useState, useRef, useCallback, useLayoutEffect } from 'react' +import log, { isDebug } from '../utility/debug.js' +import getStateSnapshot from '../utility/getStateSnapshot.js' + +import { useState, useRef, useCallback, useLayoutEffect, useInsertionEffect } from 'react' // Creates state management functions. export default function _useState({ initialState, onRender, - itemsProperty, - USE_ITEMS_UPDATE_NO_SECOND_RENDER_OPTIMIZATION + itemsProperty }) { - // This is a utility state variable that is used to re-render the component. - // It should not be used to access the current `VirtualScroller` state. - // It's more of a "requested" `VirtualScroller` state. - // - // It will also be stale in cases when `USE_ITEMS_UPDATE_NO_SECOND_RENDER_OPTIMIZATION` - // feature is used for setting new `items` in state. - // + // This is a state variable that is used to re-render the component. + // Right after the component has finished re-rendering, + // `VirtualScroller` state gets updated from this variable. + // The reason for that is that `VirtualScroller` state must always + // correspond exactly to what's currently rendered on the screen. const [_newState, _setNewState] = useState(initialState) // This `state` reference is what `VirtualScroller` uses internally. // It's the "source of truth" on the actual `VirtualScroller` state. const state = useRef(initialState) + const getState = useCallback(() => { + return state.current + }, []) + const setState = useCallback((newState) => { state.current = newState }, []) - // Accumulates all "pending" state updates until they have been applied. - const nextState = useRef(initialState) + // Updating of the actual `VirtualScroller` state is done in a + // `useInsertionEffect()` rather than in a `useLayoutEffect()`. + // + // The reason is that using `useLayoutEffect()` would result in + // "breaking" the `` when an `itemComponent` + // called `onHeightDidChange()` from its own `useLayoutEffect()`. + // In those cases, the `itemCompoent`'s effect would run before + // the ``'s effect, resulting in + // `VirtualScroller.onItemHeightDidChange(i)` being run at a moment in time + // when the DOM has already been updated for the next `VirtualScroller` state + // but the actual `VirtualScroller` state is still a previous ("stale") one + // containing "stale" first/last shown item indexes, which would result in an + // "index out of bounds" error when `onItemHeightDidChange(i)` tries to access + // and measure the DOM element from item index `i` which doesn't already/yet exist. + // + // An example of such situation could be seen from a `VirtualScroller` debug log + // which was captured for a case when using `useLayoutEffect()` to update the + // "actual" `VirtualScroller` state after the corresponding DOM changes have been applied: - // Updates the actual `VirtualScroller` state right after a requested state update - // has been applied. Doesn't do anything at initial render. - useLayoutEffect(() => { + // The user has scrolled far enough: perform a re-layout + // ~ Update Layout (on scroll) ~ + // + // Item index 2 height is required for calculations but hasn't been measured yet. Mark the item as "shown", rerender the list, measure the item's height and redo the layout. + // + // ~ Calculated Layout ~ + // Columns count 1 + // First shown item index 2 + // Last shown item index 5 + // … + // Item heights (231) [1056.578125, 783.125, empty × 229] + // Item states (231) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, …] + // + // ~ Set state ~ + // {firstShownItemIndex: 2, lastShownItemIndex: 5, …} + // + // ~ Rendered ~ + // State {firstShownItemIndex: 2, lastShownItemIndex: 5, …} + // + // ~ Measure item heights ~ + // Item index 2 height 719.8828125 + // Item index 3 height 961.640625 + // Item index 4 height 677.6640625 + // Item index 5 height 1510.1953125 + // + // ~ Update Layout (on non-measured item heights have been measured) ~ + // + // ~ Calculated Layout ~ + // Columns count 1 + // First shown item index 4 + // Last shown item index 5 + // … + // Item heights (231) [1056.578125, 783.125, 719.8828125, 961.640625, 677.6640625, 1510.1953125, empty × 225] + // Item states (231) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, …] + // + // ~ Set state ~ + // {firstShownItemIndex: 4, lastShownItemIndex: 5, beforeItemsHeight: 3521.2265625, afterItemsHeight: 214090.72265624942} + // + // ~ On Item Height Did Change was called ~ + // Item index 5 + // ~ Re-measure item height ~ + // ERROR "onItemHeightDidChange()" has been called for item index 5 but the item is not currently rendered and can't be measured. The exact error was: Element with index 3 was not found in the list of Rendered Item Elements in the Items Container of Virtual Scroller. There're only 2 Elements there. + // + // React: ~ The requested state is about to be applied in DOM. Set it as the `VirtualScroller` state. ~ + // {firstShownItemIndex: 4, lastShownItemIndex: 5, …} + // + // ~ Rendered ~ + + // "~ Rendered ~" is what gets output when `onRender()` function gets called. + // It means that `useLayoutEffect()` was triggered after `onItemHeightDidChange(i)` + // was called and after the "ERROR" happened. + // + // The "ERROR" happened because new item indexes 4…5 were actually rendered instead of + // item indexes 2…5 by the time the application called `onItemHeightDidChange(i)` function + // inside `itemComponent`'s `useLayoutEffect()`. + // Item indexes 4…5 is what was requested in a `setState()` call, which called `_setNewState()`. + // This means that `_newState` changes have been applied to the DOM + // but `useLayoutEffect()` wasn't triggered immediately after that. + // Instead, it was triggered a right after the `itemComponent`'s `useLayoutEffect()` + // because child effects run before parent effects. + // So, the `itemComponent`'s `onHeightDidChange()` function call caught the + // `VirtualScroller` in an inconsistent state. + // + // To fix that, `useLayoutEffect()` gets replaced with `useInsertionEffect()`: + // https://blog.saeloun.com/2022/06/02/react-18-useinsertioneffect + // https://beta.reactjs.org/reference/react/useInsertionEffect + // + // After replacing `useLayoutEffect()` with `useInsertionEffect()`, + // the log shows that there's no more error: + // + // ~ Set state ~ + // {firstShownItemIndex: 0, lastShownItemIndex: 2, …} + // + // React: ~ The requested state is about to be applied in DOM. Set it as the `VirtualScroller` state. ~ + // {firstShownItemIndex: 0, lastShownItemIndex: 2, …} + // + // ~ On Item Height Did Change was called ~ + // Item index 0 + // ~ Re-measure item height ~ + // Previous height 917 + // New height 1064.453125 + // ~ Item height has changed ~ + // + // An alternative solution would be demanding the `itemComponent` to + // accept a `ref` and then measuring the corresponding DOM element height + // directly using the `ref`-ed DOM element rather than searching for that + // DOM element in the `ItemsContainer`. + // So if `useInsertionEffect()` gets removed from React in some hypothetical future, + // it could be replaced with using `ref`s on `ItemComponent`s to measure the DOM element heights. + // + useInsertionEffect(() => { + // Update the actual `VirtualScroller` state right before the DOM changes + // are going to be applied for the requested state update. + // + // This hook will run right before `useLayoutEffect()`. + // + // It doesn't make any difference which one of the two hooks to use to update + // the actual `VirtualScroller` state in this scenario because the two hooks + // run synchronously one right after another (insertion effect → DOM update → layout effect) + // without any free space for any `VirtualScroller` code (like the scroll event handler) + // to squeeze in and run in-between them, so the `VirtualScroller`'s `state` + // is always gonna stay consistent with what's currently rendered on screen + // from the `VirtualScroler`'s point of view, and the short transition period + // it simply doesn't see because it doesn't "wake up" during that period. + // + // Updating the actual `VirtualScroller` state right before `useLayoutEffect()` + // fixes the bug when an `itemComponent` calls `onHeightDidChange()` in its own + // `useLayoutEffect()` which would run before this `useLayoutEffect()` + // because children's effects run before parent's. + // + // This hook doesn't do anything at the initial render. + // + if (isDebug()) { + log('React: ~ The requested state is about to be applied in DOM. Set it as the `VirtualScroller` state. ~') + log(getStateSnapshot(_newState)) + } setState(_newState) - }, [ - _newState - ]) + }, [_newState]) - // Calls `onRender()` right after every state update (which is a re-render), - // and also right after the initial render. useLayoutEffect(() => { + // Call `onRender()` right after a requested state update has been applied, + // and also right after the initial render. onRender() - }, [ - _newState, - // When using `USE_ITEMS_UPDATE_NO_SECOND_RENDER_OPTIMIZATION` feature, - // there won't be a `_setNewState()` function call when `items` property changes, - // hence the additional `itemsProperty` dependency. - USE_ITEMS_UPDATE_NO_SECOND_RENDER_OPTIMIZATION ? itemsProperty : undefined - ]) + }, [_newState]) return { - getState: () => state.current, + // This is the state the component should render. + stateToRender: _newState, - getNextState: () => nextState.current, + // Returns the current state of the `VirtualScroller`. + // This function is used in the `VirtualScroller` itself + // because the `state` is managed outside of it. + getState, // Requests a state update. - // - // State updates are incremental meaning that this function mimicks - // the classic `React.Component`'s `this.setState()` behavior - // when calling `this.setState()` didn't replace `state` but rather merged - // the updated state properties over the "old" state properties. - // - // The reason for using pending state updates accumulation is that - // `useState()` updates are "asynchronous" (not immediate), - // and simply merging over `...state` would merge over potentially stale - // property values in cases when more than a single `updateState()` call is made - // before the state actually updates, resulting in losing some of those state updates. - // - // Example: the first `updateState()` call updates shown item indexes, - // and the second `updateState()` call updates `verticalSpacing`. - // If it was simply `updateState({ ...state, ...stateUpdate })` - // then the second state update could overwrite the first state update, - // resulting in incorrect items being shown/hidden. - // - updateState: (stateUpdate) => { - nextState.current = { - ...nextState.current, - ...stateUpdate - } - // If `items` property did change, the component detects it at render time - // and updates `VirtualScroller` items immediately by calling `.setItems()`, - // which, in turn, immediately calls this `updateState()` function - // with a `stateUpdate` argument that contains the new `items`, - // so checking for `stateUpdate.items` could detect situations like that. - // - // When the initial `VirtualScroller` state is being set, it contains the `.items` - // property too, but that initial setting is done using another function called - // `setInitialState()`, so using `if (stateUpdate.items)` condition here for describing - // just the case when `state` has been updated as a result of a `setItems()` call - // seems to be fine. - // - const _newState = nextState.current - if (stateUpdate.items && USE_ITEMS_UPDATE_NO_SECOND_RENDER_OPTIMIZATION) { - setState(_newState) - } else { - _setNewState(_newState) - } - } + setState: _setNewState } } \ No newline at end of file diff --git a/source/react/useStyle.js b/source/react/useStyle.js index b4de696..75d9427 100644 --- a/source/react/useStyle.js +++ b/source/react/useStyle.js @@ -2,7 +2,7 @@ import px from '../utility/px.js' export default function useStyle({ tbody, - getNextState + state }) { if (tbody) { return @@ -11,7 +11,7 @@ export default function useStyle({ const { beforeItemsHeight, afterItemsHeight - } = getNextState() + } = state return { paddingTop: px(beforeItemsHeight), diff --git a/source/react/useUpdateItemKeysOnItemsChange.js b/source/react/useUpdateItemKeysOnItemsChange.js new file mode 100644 index 0000000..09d855b --- /dev/null +++ b/source/react/useUpdateItemKeysOnItemsChange.js @@ -0,0 +1,57 @@ +import log from '../utility/debug.js' + +import useOnChange from './useOnChange.js' + +// If the order of the `items` changes, or new `items` get prepended resulting in a "shift": +// +// * Re-generate the React `key` prefix for item elements +// so that all item components are re-rendered for the new `items` list. +// That's because item components may have their own internal state, +// and simply passing another `item` property for an item component +// might result in bugs, which React would do with its "re-using" policy +// if the unique `key` workaround hasn't been used. +// +export default function useUpdateItemKeysOnItemsChange(itemsBeingRendered, { + virtualScroller, + usesAutogeneratedItemKeys, + updateItemKeysForNewItems +}) { + // Update item keys if the items being rendered have changed. + useOnChange(itemsBeingRendered, (itemsBeingRendered, previousItemsBeingRendered) => { + if (!usesAutogeneratedItemKeys) { + return + } + + log('React: ~ Different `items` are about to be rendered', itemsBeingRendered) + + let shouldUpdateItemKeys = true + + const itemsDiff = virtualScroller.getItemsDiff(previousItemsBeingRendered, itemsBeingRendered) + + // `itemsDiff` will be `undefined` in case of a non-incremental items list change. + if (itemsDiff) { + const { + prependedItemsCount, + appendedItemsCount + } = itemsDiff + if (prependedItemsCount === 0 && appendedItemsCount === 0) { + log('React: ~ The `items` elements to be rendered are identical to the previously rendered ones') + // The items order hasn't changed. + // No need to re-generate the `key` prefix. + shouldUpdateItemKeys = false + } + else if (prependedItemsCount === 0 && appendedItemsCount > 0) { + log('React: ~ The `items` elements order hasn\'t changed:', appendedItemsCount, 'items have been appended') + // The item order hasn't changed. + // No need to re-generate the `key` prefix. + shouldUpdateItemKeys = false + } + } + + // Update React element `key`s for the new set of `items`. + if (shouldUpdateItemKeys) { + log('React: ~ Update item `key`s') + updateItemKeysForNewItems() + } + }) +} \ No newline at end of file diff --git a/source/test/ItemsContainer.js b/source/test/ItemsContainer.js index a15bcc7..6062406 100644 --- a/source/test/ItemsContainer.js +++ b/source/test/ItemsContainer.js @@ -1,3 +1,5 @@ +import ItemNotRenderedError from '../ItemNotRenderedError.js' + export default class ItemsContainer { /** * Constructs a new "container" from an element. @@ -16,9 +18,18 @@ export default class ItemsContainer { const children = this.getElement().children const maxWidth = this.getElement().width let topOffset = this.getElement().paddingTop + let rowWidth let rowHeight let startNewRow = true + + if (renderedElementIndex > children.length - 1) { + throw new ItemNotRenderedError({ + renderedElementIndex, + renderedElementsCount: children.length + }) + } + let i = 0 while (i <= renderedElementIndex) { if (startNewRow || rowWidth + children[i].width > maxWidth) { @@ -39,6 +50,7 @@ export default class ItemsContainer { } i++ } + return topOffset } @@ -48,7 +60,16 @@ export default class ItemsContainer { * @return {number} */ getNthRenderedItemHeight(renderedElementIndex) { - return this.getElement().children[renderedElementIndex].height + const children = this.getElement().children + + if (renderedElementIndex > children.length - 1) { + throw new ItemNotRenderedError({ + renderedElementIndex, + renderedElementsCount: children.length + }) + } + + return children[renderedElementIndex].height } /** diff --git a/source/utility/debug.js b/source/utility/debug.js index f668290..d3e65cf 100644 --- a/source/utility/debug.js +++ b/source/utility/debug.js @@ -13,12 +13,17 @@ export function warn(...args) { } } +function error(...args) { + console.error(...['[virtual-scroller]'].concat(args)) +} + export function reportError(...args) { + const createError = () => new Error(['[virtual-scroller]'].concat(args).join(' ')) if (typeof window !== 'undefined') { // In a web browser. // Output a debug message immediately so that it's known // at which point did the error occur between other debug logs. - log.apply(this, ['ERROR'].concat(args)) + error.apply(this, ['ERROR'].concat(args)) setTimeout(() => { // Throw an error in a timeout so that it doesn't interrupt the application's flow. // At the same time, by throwing a client-side error, such error could be spotted @@ -26,11 +31,20 @@ export function reportError(...args) { // in the console. // The `.join(' ')` part doesn't support stringifying JSON objects, // but those don't seem to be used in any of the error messages. - throw new Error(['[virtual-scroller]'].concat(args).join(' ')) + throw createError() }, 0) } else { - // On a server. - console.error(...['[virtual-scroller]'].concat(args)) + // In Node.js. + // If tests are being run, throw in case of any errors. + const catchError = getGlobalVariable('VirtualScrollerCatchError') + if (catchError) { + return catchError(createError()) + } + if (getGlobalVariable('VirtualScrollerThrowErrors')) { + throw createError() + } + // Print the error in the console. + error.apply(this, ['ERROR'].concat(args)) } } diff --git a/test/VirtualScroller.restoreState.differentColumnsCount.test.js b/test/VirtualScroller.restoreState.differentColumnsCount.test.js index 1fa69f1..e1d3aca 100644 --- a/test/VirtualScroller.restoreState.differentColumnsCount.test.js +++ b/test/VirtualScroller.restoreState.differentColumnsCount.test.js @@ -18,6 +18,9 @@ describe('VirtualScroller', function() { // 16 items, 8 rows. const items = new Array(ROWS_COUNT * COLUMNS_COUNT).fill({ area: ITEM_WIDTH * ITEM_HEIGHT }) + const errors = [] + global.VirtualScrollerCatchError = (error) => errors.push(error) + const virtualScroller = new VirtualScroller({ // The `items` option will be overridden by the `items` from the restored state. items: [], @@ -38,6 +41,11 @@ describe('VirtualScroller', function() { } }) + global.VirtualScrollerCatchError = undefined + errors.length.should.equal(2) + errors[0].message.should.equal('[virtual-scroller] ~ Columns Count changed from 1 to 2 ~') + errors[1].message.should.equal('[virtual-scroller] Reset Layout') + // Columns count mismatch — the state gets reset. virtualScroller.verifyState({ firstShownItemIndex: 0, diff --git a/test/VirtualScroller.setItems.prependItemsCountNonDivisibleByColumnsCount.test.js b/test/VirtualScroller.setItems.prependItemsCountNonDivisibleByColumnsCount.test.js index d686f51..b98de19 100644 --- a/test/VirtualScroller.setItems.prependItemsCountNonDivisibleByColumnsCount.test.js +++ b/test/VirtualScroller.setItems.prependItemsCountNonDivisibleByColumnsCount.test.js @@ -82,10 +82,18 @@ describe('VirtualScroller', function() { itemStates: new Array(items.length) }) + const errors = [] + global.VirtualScrollerCatchError = (error) => errors.push(error) + virtualScroller.setItems(items, { preserveScrollPositionOnPrependItems: true }) + global.VirtualScrollerCatchError = undefined + errors.length.should.equal(2) + errors[0].message.should.equal('[virtual-scroller] ~ Prepended items count 1 is not divisible by Columns Count 2 ~') + errors[1].message.should.equal('[virtual-scroller] Layout reset required') + // Stop listening to scroll events. virtualScroller.stop() }) diff --git a/test/setup.js b/test/setup.js index d13f76b..f02c0b4 100644 --- a/test/setup.js +++ b/test/setup.js @@ -4,4 +4,5 @@ global.expect = expect chai.should() global.VirtualScrollerDebug = true -global.VirtualScrollerWarningsAreErrors = true \ No newline at end of file +global.VirtualScrollerWarningsAreErrors = true +global.VirtualScrollerThrowErrors = true \ No newline at end of file