Repository: mapbox/mr-ui
Pull request #213 (2023-08-17 12:01:05)

BILLING-576: MinimumDurationLoader mounts twice if loading finishes before MinDuration has passed
Fixes a bug that happens when `isLoaded` is set to `true` before `minDuration` has passed. In this scenario, the `children` will get mounted twice.

I observed that `shouldRenderContent` essentially flip-flops between `true` and `false` causing unnecessary unmount / remount. The relevant pieces of state look like this
```
Initial: isLoaded=false, wasLoaded=false, delayedLoaded=false          -> shouldRenderContent=false
First mount: isLoaded=true, wasLoaded=false, delayedLoaded=true        -> shouldRenderContent=true
Immediately after: isLoaded=true, wasLoaded=false, delayedLoaded=false -> shouldRenderContent=false
Followed by: isLoaded=true, wasLoaded=false, delayedLoaded=true        -> shouldRenderContent=true
```

The best fix here would most likely be the figure out what causes this flip-flopping. However, the component is quite difficult to follow and the intended behavior seems to be to render the `children` exactly once anyway. Thus, I fixed this by considering `prevIsLoaded` too when deciding the value for `shouldRenderContent`

In most cases, this is probably at most a small performance issue but this bug surfaced when Billing team noticed that account dashboard actually has some components with side-effects during mount (e.g. when verifying email address). Since we depend quite heavily on the `MinimumDurationLoader`, I figured that fixing the bug here makes sense, despite having side-effects occur during mount is also pretty dirty.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e3d0711..f7b1414 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 ## Changelog
 
+## 2.6.2
+
+- [bug] `MinimumDurationLoader` mounts the underlying components twice if `minDuration` is reached before `isLoaded` is set to `true`.
+
 ## 2.6.1
 
 - [fix] Incorrect directory was published to npm as 2.6.0
diff --git a/package-lock.json b/package-lock.json
index 17e75e2..89fc0d1 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,12 +1,12 @@
 {
   "name": "@mapbox/mr-ui",
-  "version": "2.6.1",
+  "version": "2.6.2",
   "lockfileVersion": 2,
   "requires": true,
   "packages": {
     "": {
       "name": "@mapbox/mr-ui",
-      "version": "2.6.1",
+      "version": "2.6.2",
       "license": "BSD-2-Clause",
       "dependencies": {
         "@mapbox/mbx-assembly": "^1.4.0",
diff --git a/package.json b/package.json
index 3a5cb0f..23a81b4 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@mapbox/mr-ui",
-  "version": "2.6.1",
+  "version": "2.6.2",
   "description": "UI components for Mapbox projects",
   "main": "index.js",
   "homepage": "./",
diff --git a/src/components/minimum-duration-loader/minimum-duration-loader.tsx b/src/components/minimum-duration-loader/minimum-duration-loader.tsx
index eb02553..0f777c3 100644
--- a/src/components/minimum-duration-loader/minimum-duration-loader.tsx
+++ b/src/components/minimum-duration-loader/minimum-duration-loader.tsx
@@ -65,7 +65,7 @@ export default function MinimumDurationLoader({
     isLoaded
   ]);
 
-  const shouldRenderContent = isLoaded && (wasLoaded || delayedLoaded);
+  const shouldRenderContent = isLoaded && (wasLoaded || delayedLoaded || prevIsLoaded);
 
   if (shouldRenderContent) {
     return (


Test patch:
diff --git a/src/components/minimum-duration-loader/minimum-duration-loader.test.tsx b/src/components/minimum-duration-loader/minimum-duration-loader.test.tsx
index 29477b4..17afd9a 100644
--- a/src/components/minimum-duration-loader/minimum-duration-loader.test.tsx
+++ b/src/components/minimum-duration-loader/minimum-duration-loader.test.tsx
@@ -1,5 +1,5 @@
 import React from 'react';
-import { render, screen } from '@testing-library/react';
+import { render, screen, act } from '@testing-library/react';
 import MinimumDurationLoader from './minimum-duration-loader';
 
 const children = <span data-testid='test-content'>Content</span>;
@@ -37,7 +37,7 @@ describe('MinimumDurationLoader', () => {
       expect(screen.getByTestId('minimum-duration-loader')).toBeInTheDocument();
       expect(screen.queryByTestId('test-content')).not.toBeInTheDocument();
 
-      jest.advanceTimersByTime(1000);
+     act(() => jest.advanceTimersByTime(1000));
 
       expect(screen.queryByTestId('minimum-duration-loader')).not.toBeInTheDocument();
       expect(screen.getByTestId('test-content')).toBeInTheDocument();
@@ -46,11 +46,11 @@ describe('MinimumDurationLoader', () => {
     test.skip('does not render the content if isLoaded becomes `false` after having been `true`', () => {
       const { rerender } = render(<MinimumDurationLoader {...{...props, isLoaded: true}} />)
       expect(screen.getByTestId('minimum-duration-loader')).toBeInTheDocument();
-      jest.advanceTimersByTime(1000);
+      act(() => jest.advanceTimersByTime(1000));
       expect(screen.getByTestId('test-content')).toBeInTheDocument();
       rerender(<MinimumDurationLoader {...props } />)
       expect(screen.getByTestId('minimum-duration-loader')).toBeInTheDocument();
-      jest.advanceTimersByTime(1000);
+      act(() => jest.advanceTimersByTime(1000));
       expect(screen.getByTestId('test-content')).toBeInTheDocument();
     });
   });
@@ -104,11 +104,11 @@ describe('MinimumDurationLoader', () => {
       // Loader should exist
       expect(screen.getByTestId('minimum-duration-loader')).toBeInTheDocument();
       expect(screen.queryByTestId('test-content')).not.toBeInTheDocument();
-      jest.advanceTimersByTime(1000);
+      act(() => jest.advanceTimersByTime(1000));
       // Loader should still exist because we exceeded the minimum duration
       expect(screen.getByTestId('minimum-duration-loader')).toBeInTheDocument();
       expect(screen.queryByTestId('test-content')).not.toBeInTheDocument();
-      jest.advanceTimersByTime(4000);
+      act(() => jest.advanceTimersByTime(4000));
       // We've exceeded the minimum duration, so the loader should not exist
       expect(screen.queryByTestId('minimum-duration-loader')).not.toBeInTheDocument();
       expect(screen.getByTestId('test-content')).toBeInTheDocument();
@@ -164,4 +164,102 @@ describe('MinimumDurationLoader', () => {
       expect(baseElement).toMatchSnapshot();
     });
   });
+
+  describe('always mounts the content exactly once', () => {
+    beforeEach(() => {
+      jest.useFakeTimers();
+    });
+
+    afterEach(() => {
+      jest.useRealTimers();
+    });
+
+    let mountedCount = 0;
+
+    const Counter: React.FC = () => {
+      React.useEffect(() => {
+        mountedCount++
+      }, []);
+
+      return <span data-testid="mounted-count">{mountedCount}</span>;
+    }
+
+    afterEach(() => {
+      mountedCount = 0;
+    })
+
+    test('when isLoaded = true before minDuration has passed', () => {
+      const MIN_DURATION = 100;
+      const { rerender } = render(
+        <MinimumDurationLoader minDuration={MIN_DURATION} isLoaded={false}>
+          <Counter />
+        </MinimumDurationLoader>
+      )
+
+      expect(mountedCount).toBe(0)
+
+      act(() => jest.advanceTimersByTime(60));
+      rerender(
+        <MinimumDurationLoader minDuration={MIN_DURATION} isLoaded={true}>
+          <Counter />
+        </MinimumDurationLoader>
+      )
+
+      // Does not mount yet since minDuration has not been reached
+      expect(mountedCount).toBe(0)
+      rerender(
+        <MinimumDurationLoader minDuration={MIN_DURATION} isLoaded={true}>
+          <Counter />
+        </MinimumDurationLoader>
+      )
+
+      act(() => jest.advanceTimersByTime(60));
+
+      // Mounts since minDuration has passed and isLoaded = true
+      expect(mountedCount).toBe(1)
+      
+      rerender(
+        <MinimumDurationLoader minDuration={MIN_DURATION} isLoaded={true}>
+          <Counter />
+        </MinimumDurationLoader>
+      )
+
+      // Confirm that the component has not been mounted again if we move forward in time
+      act(() => jest.advanceTimersByTime(1));
+      expect(mountedCount).toBe(1)
+    });
+
+    test('when isLoaded = true after minDuration has passed', () => {
+      const MIN_DURATION = 100;
+      const { rerender } = render(
+        <MinimumDurationLoader minDuration={MIN_DURATION} isLoaded={false}>
+          <Counter />
+        </MinimumDurationLoader>
+      )
+
+      expect(mountedCount).toBe(0)
+
+      // Move forward in time more than minDuration before setting isLoaded = true
+      act(() => jest.advanceTimersByTime(110));      
+      rerender(
+        <MinimumDurationLoader minDuration={MIN_DURATION} isLoaded={true}>
+          <Counter />
+        </MinimumDurationLoader>
+      )
+
+      // Mounts since minDuration has passed and isLoaded = true
+      expect(mountedCount).toBe(1)
+
+      // Confirm that the component has not been mounted again if we move forward in time
+      act(() => jest.advanceTimersByTime(1));
+      rerender(
+        <MinimumDurationLoader minDuration={MIN_DURATION} isLoaded={true}>
+          <Counter />
+        </MinimumDurationLoader>
+      )
+
+
+      expect(mountedCount).toBe(1)
+    });
+  })
 });