refactor!: split getItem into smaller methods, set loading state in updateRow (#9698)

vursen · web-flow · commit 865be9b252a9 · 2025-07-11T16:51:21.000+03:00
diff --git a/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js b/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js
@@ -543,10 +543,10 @@ export const InlineEditingMixin = (superClass) =>
 
     /**
      * @param {!HTMLElement} row
-     * @param {GridItem} item
      * @private
      */
-    __updateRow(row, item = row._item) {
+    __updateRow(row) {
+      const item = this.__getRowItem(row);
       if (this.__edited) {
         const { cell, model } = this.__edited;
         if (cell.parentNode === row && model.item !== item) {
diff --git a/packages/grid/src/vaadin-grid-data-provider-mixin.js b/packages/grid/src/vaadin-grid-data-provider-mixin.js
@@ -7,7 +7,6 @@ import { microTask, timeOut } from '@vaadin/component-base/src/async.js';
 import { DataProviderController } from '@vaadin/component-base/src/data-provider-controller/data-provider-controller.js';
 import { Debouncer } from '@vaadin/component-base/src/debounce.js';
 import { get } from '@vaadin/component-base/src/path-utils.js';
-import { getBodyRowCells, updateCellsPart, updateState } from './vaadin-grid-helpers.js';
 
 /**
  * @polymerMixin
@@ -182,46 +181,20 @@ export const DataProviderMixin = (superClass) =>
       this.requestContentUpdate();
     }
 
-    /**
-     * @param {number} index
-     * @param {HTMLElement} row
-     * @protected
-     */
-    _getItem(index, row) {
-      row.index = index;
-
-      const { item } = this._dataProviderController.getFlatIndexContext(index);
-      if (item) {
-        this.__updateLoading(row, false);
-        this.__updateRow(row, item);
-        if (this._isExpanded(item)) {
-          this._dataProviderController.ensureFlatIndexHierarchy(index);
-        }
-      } else {
-        this.__updateLoading(row, true);
-        this._dataProviderController.ensureFlatIndexLoaded(index);
-      }
+    /** @private */
+    __getRowItem(row) {
+      const { item } = this._dataProviderController.getFlatIndexContext(row.index);
+      return item;
     }
 
-    /**
-     * @param {!HTMLElement} row
-     * @param {boolean} loading
-     * @private
-     */
-    __updateLoading(row, loading) {
-      const cells = getBodyRowCells(row);
-
-      // Row state attribute
-      updateState(row, 'loading', loading);
-
-      // Cells part attribute
-      updateCellsPart(cells, 'loading-row-cell', loading);
+    /** @private */
+    __ensureRowItem(row) {
+      this._dataProviderController.ensureFlatIndexLoaded(row.index);
+    }
 
-      if (loading) {
-        // Run style generators for the loading row to have custom names cleared
-        this._generateCellClassNames(row);
-        this._generateCellPartNames(row);
-      }
+    /** @private */
+    __ensureRowHierarchy(row) {
+      this._dataProviderController.ensureFlatIndexHierarchy(row.index);
     }
 
     /**
@@ -302,7 +275,7 @@ export const DataProviderMixin = (superClass) =>
       if (this._flatSize !== this._dataProviderController.flatSize) {
         // Schedule an update of all rendered rows by _debouncerApplyCachedData,
         // to ensure that all pages associated with the rendered rows are loaded.
-        this._shouldUpdateAllRenderedRowsAfterPageLoad = true;
+        this._shouldLoadAllRenderedRowsAfterPageLoad = true;
 
         // TODO: Updating the flat size property can still result in a synchonous virtualizer update
         // if the size change requires the virtualizer to increase the amount of physical elements
@@ -312,9 +285,7 @@ export const DataProviderMixin = (superClass) =>
       }
 
       // After updating the cache, check if some of the expanded items should have sub-caches loaded
-      this._getRenderedRows().forEach((row) => {
-        this._dataProviderController.ensureFlatIndexHierarchy(row.index);
-      });
+      this._getRenderedRows().forEach((row) => this.__ensureRowHierarchy(row));
 
       this._hasData = true;
     }
@@ -325,13 +296,14 @@ export const DataProviderMixin = (superClass) =>
       this._debouncerApplyCachedData = Debouncer.debounce(this._debouncerApplyCachedData, timeOut.after(0), () => {
         this._setLoading(false);
 
-        const shouldUpdateAllRenderedRowsAfterPageLoad = this._shouldUpdateAllRenderedRowsAfterPageLoad;
-        this._shouldUpdateAllRenderedRowsAfterPageLoad = false;
+        const shouldLoadAllRenderedRowsAfterPageLoad = this._shouldLoadAllRenderedRowsAfterPageLoad;
+        this._shouldLoadAllRenderedRowsAfterPageLoad = false;
 
         this._getRenderedRows().forEach((row) => {
-          const { item } = this._dataProviderController.getFlatIndexContext(row.index);
-          if (item || shouldUpdateAllRenderedRowsAfterPageLoad) {
-            this._getItem(row.index, row);
+          this.__updateRow(row);
+
+          if (shouldLoadAllRenderedRowsAfterPageLoad) {
+            this.__ensureRowItem(row);
           }
         });
 
diff --git a/packages/grid/src/vaadin-grid-mixin.js b/packages/grid/src/vaadin-grid-mixin.js
@@ -28,6 +28,7 @@ import {
   iterateRowCells,
   updateBooleanRowStates,
   updateCellsPart,
+  updateState,
 } from './vaadin-grid-helpers.js';
 import { KeyboardNavigationMixin } from './vaadin-grid-keyboard-navigation-mixin.js';
 import { RowDetailsMixin } from './vaadin-grid-row-details-mixin.js';
@@ -639,10 +640,15 @@ export const GridMixin = (superClass) =>
         return;
       }
 
+      row.index = index;
+
       this._updateRowOrderParts(row, index);
 
       this._a11yUpdateRowRowindex(row, index);
-      this._getItem(index, row);
+
+      this.__ensureRowItem(row);
+      this.__ensureRowHierarchy(row);
+      this.__updateRow(row);
     }
 
     /** @private */
@@ -741,10 +747,38 @@ export const GridMixin = (superClass) =>
 
     /**
      * @param {!HTMLElement} row
-     * @param {GridItem} item
+     * @param {boolean} loading
+     * @private
+     */
+    __updateRowLoading(row, loading) {
+      const cells = getBodyRowCells(row);
+
+      // Row state attribute
+      updateState(row, 'loading', loading);
+
+      // Cells part attribute
+      updateCellsPart(cells, 'loading-row-cell', loading);
+
+      if (loading) {
+        // Run style generators for the loading row to have custom names cleared
+        this._generateCellClassNames(row);
+        this._generateCellPartNames(row);
+      }
+    }
+
+    /**
+     * @param {!HTMLElement} row
      * @private
      */
-    __updateRow(row, item = row._item) {
+    __updateRow(row) {
+      const item = this.__getRowItem(row);
+      if (item) {
+        this.__updateRowLoading(row, false);
+      } else {
+        this.__updateRowLoading(row, true);
+        return;
+      }
+
       row._item = item;
       const model = this.__getRowModel(row);
 
diff --git a/packages/grid/test/data-provider.test.js b/packages/grid/test/data-provider.test.js
@@ -606,13 +606,7 @@ describe('data provider', () => {
     });
 
     describe('rendering', () => {
-      function getFirstRowUpdateCount() {
-        const callsForFirstIndex = grid.__updateRow.getCalls().filter((call) => {
-          const item = call.args[1];
-          return item.value === '0';
-        });
-        return callsForFirstIndex.length;
-      }
+      let firstRowRendererSpy;
 
       beforeEach(async () => {
         grid.itemIdPath = 'value';
@@ -624,40 +618,47 @@ describe('data provider', () => {
             };
           });
 
-          cb(pageItems, 3);
+          cb(pageItems, pageItems.length);
+        };
+        grid.querySelector('vaadin-grid-column').renderer = (_root, _grid, { item }) => {
+          if (item.value === '0') {
+            firstRowRendererSpy?.();
+          }
         };
-        sinon.spy(grid, '__updateRow');
+
         await nextFrame();
+
+        firstRowRendererSpy = sinon.spy();
       });
 
       it('should limit row updates', async () => {
         grid.expandedItems = [{ value: '0' }, { value: '1' }, { value: '1-0' }, { value: '2' }];
         await nextFrame();
         // There are currently two __updateRow calls for a row. The extra one (a direct update request)
         // is coming from _expandedItemsChanged.
-        expect(getFirstRowUpdateCount()).to.equal(2);
+        expect(firstRowRendererSpy).to.have.callCount(4);
       });
 
       it('should limit row updates on a small size', async () => {
         grid.size = 3;
         grid.expandedItems = [{ value: '0' }, { value: '1' }, { value: '1-0' }, { value: '2' }];
         await nextFrame();
-        expect(getFirstRowUpdateCount()).to.equal(2);
+        expect(firstRowRendererSpy).to.have.callCount(4);
       });
 
       it('should limit row updates on a small page size', async () => {
         grid.pageSize = 1;
         grid.expandedItems = [{ value: '0' }, { value: '1' }, { value: '1-0' }, { value: '2' }];
         await nextFrame();
         // Changing page size causes yet an additional update request
-        expect(getFirstRowUpdateCount()).to.equal(3);
+        expect(firstRowRendererSpy).to.have.callCount(6);
       });
 
       it('should limit row updates on a small page size, reverse property update order', async () => {
         grid.expandedItems = [{ value: '0' }, { value: '1' }, { value: '1-0' }, { value: '2' }];
         grid.pageSize = 1;
         await nextFrame();
-        expect(getFirstRowUpdateCount()).to.equal(3);
+        expect(firstRowRendererSpy).to.have.callCount(6);
       });
     });
   });
