Merge pull request #236 from inokawa/fix-235

Additional fix for #235
inokawa · Oct 31, 2023 · 3cb0516 · 3cb0516
2 parents 9ab9265 + d7ec820
commit 3cb0516
Show file tree

Hide file tree

Showing 13 changed files with 1,499 additions and 340 deletions.
diff --git a/src/core/store.ts b/src/core/store.ts
@@ -110,7 +110,6 @@ export const createVirtualStore = (
   let _smoothScrollRange: ItemsRange | null = null;
   let _maybeJumped = false;
   let _prevRange: ItemsRange = [0, initialItemCount];
-  let _initialRendered = true;
 
   const subscribers = new Set<[number, Subscriber]>();
   const getScrollSize = (): number =>
@@ -224,13 +223,9 @@ export const createVirtualStore = (
             ([index, size]) => cache._sizes[index] !== size
           );
           // Skip if all items are cached and not updated
-          if (
-            !updated.length &&
-            !_initialRendered
-            ) {
+          if (!updated.length) {
             break;
           }
-          _initialRendered = false;
 
           // Calculate jump
           // Should maintain visible position to minimize junks in appearance

diff --git a/src/react/VGrid.tsx b/src/react/VGrid.tsx
@@ -273,16 +273,14 @@ export const VGrid = forwardRef<VGridHandle, VGridProps>(
 
     useIsomorphicLayoutEffect(() => {
       const root = rootRef[refKey]!;
-      const cleanUpResizer = resizer._observeRoot(root);
-      const cleanupVScroller = vScroller._initRoot(root);
-      const cleanupHScroller = hScroller._initRoot(root);
       const onRerender = (sync?: boolean) => {
         if (sync) {
           flushSync(rerender);
         } else {
           rerender();
         }
       };
+      // store must be subscribed first because others may dispatch update on init depending on implementation
       const unsubscribeVStore = vStore._subscribe(
         UPDATE_SCROLL_STATE + UPDATE_SIZE_STATE,
         onRerender
@@ -291,12 +289,15 @@ export const VGrid = forwardRef<VGridHandle, VGridProps>(
         UPDATE_SCROLL_STATE + UPDATE_SIZE_STATE,
         onRerender
       );
+      const cleanUpResizer = resizer._observeRoot(root);
+      const cleanupVScroller = vScroller._initRoot(root);
+      const cleanupHScroller = hScroller._initRoot(root);
       return () => {
+        unsubscribeVStore();
+        unsubscribeHStore();
         cleanUpResizer();
         cleanupVScroller();
         cleanupHScroller();
-        unsubscribeVStore();
-        unsubscribeHStore();
       };
     }, []);
 

diff --git a/src/react/VList.spec.tsx b/src/react/VList.spec.tsx
@@ -104,7 +104,7 @@ describe("render count", () => {
       </Profiler>
     );
 
-    expect(rootFn).toBeCalledTimes(1);
+    expect(rootFn).toBeCalledTimes(2);
     itemFns.forEach((itemFn) => {
       expect(itemFn).toHaveBeenCalledTimes(1);
     });
@@ -130,7 +130,7 @@ describe("render count", () => {
       </Profiler>
     );
 
-    expect(rootFn).toBeCalledTimes(1);
+    expect(rootFn).toBeCalledTimes(3);
     itemFns.forEach((itemFn) => {
       expect(itemFn.mock.calls.length).toBeLessThanOrEqual(1);
     });
@@ -185,7 +185,7 @@ describe("render count", () => {
       </Mounter>
     );
 
-    expect(rootFn).toBeCalledTimes(1);
+    expect(rootFn).toBeCalledTimes(2);
     itemFns.forEach((itemFn, i) => {
       expect(itemFn).toBeCalledTimes(i === itemFns.length - 1 ? 0 : 1);
     });
@@ -240,7 +240,7 @@ describe("render count", () => {
       </Mounter>
     );
 
-    expect(rootFn).toBeCalledTimes(2);
+    expect(rootFn).toBeCalledTimes(3);
     itemFns.forEach((itemFn) => {
       expect(itemFn.mock.calls.length).toBeLessThanOrEqual(2); // TODO: should be 1
     });

diff --git a/src/react/VList.tsx b/src/react/VList.tsx
@@ -232,8 +232,7 @@ export const VList = forwardRef<VListHandle, VListProps>(
 
     useIsomorphicLayoutEffect(() => {
       const root = rootRef[refKey]!;
-      const cleanupResizer = resizer._observeRoot(root);
-      const cleanupScroller = scroller._initRoot(root);
+      // store must be subscribed first because others may dispatch update on init depending on implementation
       const unsubscribeStore = store._subscribe(
         UPDATE_SCROLL_STATE + UPDATE_SIZE_STATE,
         (sync) => {
@@ -250,11 +249,13 @@ export const VList = forwardRef<VListHandle, VListProps>(
           onScroll[refKey] && onScroll[refKey](store._getScrollOffset());
         }
       );
+      const cleanupResizer = resizer._observeRoot(root);
+      const cleanupScroller = scroller._initRoot(root);
       return () => {
-        cleanupResizer();
-        cleanupScroller();
         unsubscribeStore();
         unsubscribeOnScroll();
+        cleanupResizer();
+        cleanupScroller();
       };
     }, []);
 

diff --git a/src/react/WVList.spec.tsx b/src/react/WVList.spec.tsx
@@ -112,7 +112,7 @@ describe("render count", () => {
       </Profiler>
     );
 
-    expect(rootFn).toBeCalledTimes(1);
+    expect(rootFn).toBeCalledTimes(2);
     itemFns.forEach((itemFn) => {
       expect(itemFn).toHaveBeenCalledTimes(1);
     });
@@ -138,7 +138,7 @@ describe("render count", () => {
       </Profiler>
     );
 
-    expect(rootFn).toBeCalledTimes(1);
+    expect(rootFn).toBeCalledTimes(3);
     itemFns.forEach((itemFn) => {
       expect(itemFn.mock.calls.length).toBeLessThanOrEqual(1);
     });
@@ -193,7 +193,7 @@ describe("render count", () => {
       </Mounter>
     );
 
-    expect(rootFn).toBeCalledTimes(1);
+    expect(rootFn).toBeCalledTimes(2);
     itemFns.forEach((itemFn, i) => {
       expect(itemFn).toBeCalledTimes(i === itemFns.length - 1 ? 0 : 1);
     });
@@ -248,7 +248,7 @@ describe("render count", () => {
       </Mounter>
     );
 
-    expect(rootFn).toBeCalledTimes(2);
+    expect(rootFn).toBeCalledTimes(3);
     itemFns.forEach((itemFn) => {
       expect(itemFn.mock.calls.length).toBeLessThanOrEqual(2); // TODO: should be 1
     });

diff --git a/src/react/WVList.tsx b/src/react/WVList.tsx
@@ -186,8 +186,7 @@ export const WVList = forwardRef<WVListHandle, WVListProps>(
 
     useIsomorphicLayoutEffect(() => {
       const root = rootRef[refKey]!;
-      const cleanupResizer = resizer._observeRoot(root);
-      const cleanupScroller = scroller._initRoot(root);
+      // store must be subscribed first because others may dispatch update on init depending on implementation
       const unsubscribeStore = store._subscribe(
         UPDATE_SCROLL_STATE + UPDATE_SIZE_STATE,
         (sync) => {
@@ -198,10 +197,12 @@ export const WVList = forwardRef<WVListHandle, WVListProps>(
           }
         }
       );
+      const cleanupResizer = resizer._observeRoot(root);
+      const cleanupScroller = scroller._initRoot(root);
       return () => {
+        unsubscribeStore();
         cleanupResizer();
         cleanupScroller();
-        unsubscribeStore();
       };
     }, []);
 

diff --git a/src/react/__snapshots__/VGrid.rtl.spec.tsx.snap b/src/react/__snapshots__/VGrid.rtl.spec.tsx.snap
@@ -6,71 +6,120 @@ exports[`rtl should work 1`] = `
     style="overflow: auto; contain: strict; width: 100%; height: 100%;"
   >
     <div
-      style="position: relative; visibility: hidden; width: 10000px; height: 4000px; pointer-events: auto;"
+      style="position: relative; visibility: hidden; width: 10000px; height: 4040px; pointer-events: auto;"
     >
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 0px; right: 0px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 0px; right: 0px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           0 / 0
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 0px; right: 100px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 0px; right: 100px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           0 / 1
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 0px; right: 200px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 0px; right: 200px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           0 / 2
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 40px; right: 0px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 0px; right: 300px; visibility: visible; min-height: 50px; min-width: 100px;"
+      >
+        <div>
+          0 / 3
+        </div>
+      </div>
+      <div
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 50px; right: 0px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           1 / 0
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 40px; right: 100px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 50px; right: 100px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           1 / 1
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 40px; right: 200px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 50px; right: 200px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           1 / 2
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 80px; right: 0px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 50px; right: 300px; visibility: visible; min-height: 50px; min-width: 100px;"
+      >
+        <div>
+          1 / 3
+        </div>
+      </div>
+      <div
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 100px; right: 0px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           2 / 0
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 80px; right: 100px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 100px; right: 100px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           2 / 1
         </div>
       </div>
       <div
-        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 80px; right: 200px; visibility: hidden; min-height: 40px; min-width: 100px;"
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 100px; right: 200px; visibility: visible; min-height: 50px; min-width: 100px;"
       >
         <div>
           2 / 2
         </div>
       </div>
+      <div
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 100px; right: 300px; visibility: visible; min-height: 50px; min-width: 100px;"
+      >
+        <div>
+          2 / 3
+        </div>
+      </div>
+      <div
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 150px; right: 0px; visibility: visible; min-height: 50px; min-width: 100px;"
+      >
+        <div>
+          3 / 0
+        </div>
+      </div>
+      <div
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 150px; right: 100px; visibility: visible; min-height: 50px; min-width: 100px;"
+      >
+        <div>
+          3 / 1
+        </div>
+      </div>
+      <div
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 150px; right: 200px; visibility: visible; min-height: 50px; min-width: 100px;"
+      >
+        <div>
+          3 / 2
+        </div>
+      </div>
+      <div
+        style="display: grid; margin: 0px; padding: 0px; position: absolute; top: 150px; right: 300px; visibility: visible; min-height: 50px; min-width: 100px;"
+      >
+        <div>
+          3 / 3
+        </div>
+      </div>
     </div>
   </div>
 </DocumentFragment>