Upgrade React to v18 (#1093)

There were a few minor code changes needed for React 18 to work, and a *lot* of changes needed to keep tests working (most really have no major runtime impact, although a handful do probably make things slightly more robust).

A lot more stuff under the hood in React has become async, so most of these test changes are about timing, making sure a component fully renders and goes through its loading lifecycle (and cancels async API calls if the component is prematurely unloaded, as they are in some tests). Similarly, lots of test code also needed be wrapped in `act(() => doSomething())`.

I also updated some of the JS class syntax for a couple components that needed more invasive work for data/state loading while I was in there (for example, `diff-view.jsx`).

Author: Mr0grog

package-lock.json

@@ -19,7 +19,7 @@
     "@gfx/zopfli": "^1.0.15",
     "@testing-library/dom": "^10.4.0",
     "@testing-library/jest-dom": "^6.6.3",
-    "@testing-library/react": "^12.1.5",
+    "@testing-library/react": "^16.3.0",
     "autoprefixer": "^10.4.21",
     "babel-core": "^7.0.0-bridge.0",
     "babel-loader": "^9.2.1",
@@ -35,9 +35,9 @@
     "mini-css-extract-plugin": "^2.9.2",
     "postcss": "^8.5.6",
     "postcss-loader": "^8.1.1",
-    "react": "^17.0.2",
+    "react": "^18.3.1",
     "react-aria-modal": "^5.0.2",
-    "react-dom": "^17.0.2",
+    "react-dom": "^18.3.1",
     "react-router-dom": "^5.3.4",
     "react-tooltip": "^5.29.1",
     "style-loader": "^4.0.0",

package.json

@@ -34,9 +34,7 @@ exports[`PageUrlDetails Component shows separate URL histories for each version
            
           / earth-science-conference-convenes
            
-          
            
-          
         </a>
         <i
           class="fa fa-no-hover fa-right-icon fa-angle-right"
@@ -67,9 +65,7 @@ exports[`PageUrlDetails Component shows separate URL histories for each version
             / something
           </ins>
            
-          
            
-          
         </a>
       </li>
     </ol>
@@ -99,9 +95,7 @@ exports[`PageUrlDetails Component shows separate URL histories for each version
            
           / earth-science-conference-convenes
            
-          
            
-          
         </a>
         <i
           class="fa fa-no-hover fa-right-icon fa-angle-right"
@@ -136,9 +130,7 @@ exports[`PageUrlDetails Component shows separate URL histories for each version
             / else
           </ins>
            
-          
            
-          
         </a>
       </li>
     </ol>
@@ -177,9 +169,7 @@ exports[`PageUrlDetails Component shows the versions' URL if it differs from the
         / something
       </ins>
        
-      
        
-      
     </a>
   </details>
 </div>
@@ -216,9 +206,7 @@ exports[`PageUrlDetails Component shows the versions' redirects 1`] = `
            
           / earth-science-conference-convenes
            
-          
            
-          
         </a>
         <i
           class="fa fa-no-hover fa-right-icon fa-angle-right"
@@ -249,9 +237,7 @@ exports[`PageUrlDetails Component shows the versions' redirects 1`] = `
             / something
           </ins>
            
-          
            
-          
         </a>
       </li>
     </ol>

src/components/__tests__/__snapshots__/page-url-details.test.jsx.snap

@@ -1,6 +1,6 @@
 /* eslint-env jest */
 
-import { render, screen, fireEvent } from '@testing-library/react';
+import { render, screen, fireEvent, act } from '@testing-library/react';
 import { ApiContext } from '../api-context';
 import ChangeView, { defaultDiffType, diffTypeStorage } from '../change-view/change-view';
 import layeredStorage from '../../scripts/layered-storage';
@@ -86,13 +86,13 @@ describe('change-view', () => {
 
   describe('initial diffType', () => {
     describe('when a diffType has been stored in layeredStorage and is relevant to the pages being compared', () => {
-      it('sets state.diffType to the stored value', () => {
+      it('sets state.diffType to the stored value', async () => {
         const storedDiffType = 'CHANGES_ONLY_TEXT';
         layeredStorage.setItem(diffTypeStorage, storedDiffType);
 
         renderBasicChangeView({ mediaType: 'text/html' });
 
-        screen.getByText(`diffType="${storedDiffType}"`);
+        await screen.findByText(`diffType="${storedDiffType}"`);
       });
 
       describe('when a diffType has been stored in layeredStorage and is is NOT relevant to the pages being compared', () => {
@@ -222,19 +222,24 @@ describe('change-view', () => {
       // sanity check
       expect(newType).not.toEqual(defaultDiffType);
 
-      const diffSelector = screen.getByLabelText('Comparison:');
-      diffSelector.value = newType;
-      fireEvent.change(diffSelector);
+      await act(() => {
+        const diffSelector = screen.getByLabelText('Comparison:');
+        diffSelector.value = newType;
+        fireEvent.change(diffSelector);
+      });
+
       screen.getByText(`diffType="${newType}"`);
     });
 
-    it('stores the new diffType in layeredStorage',  () => {
+    it('stores the new diffType in layeredStorage',  async () => {
       renderBasicChangeView({ mediaType: 'text/html' });
 
       const newType = diffTypesFor('text/html')[0].value;
-      const diffSelector = screen.getByLabelText('Comparison:');
-      diffSelector.value = newType;
-      fireEvent.change(diffSelector);
+      await act(() => {
+        const diffSelector = screen.getByLabelText('Comparison:');
+        diffSelector.value = newType;
+        fireEvent.change(diffSelector);
+      });
 
       expect(layeredStorage.getItem(diffTypeStorage)).toBe(newType);
     });

src/components/__tests__/change-view.test.jsx

@@ -31,6 +31,8 @@ describe('diff-view', () => {
     );
 
     expect(container).not.toBeEmptyDOMElement();
+    await screen.findByText(/loading/i);
+    await waitFor(() => expect(screen.queryByText(/loading/i)).toBeNull());
   });
 
   it('renders an alert if there are no changes in the diff', async () => {
@@ -47,7 +49,7 @@ describe('diff-view', () => {
       </ApiContext.Provider>
     );
 
-    await waitFor(() => screen.getByRole('alert'));
+    await screen.findByRole('alert');
   });
 
   it('renders no alert if there are changes in the diff', async () => {
@@ -63,7 +65,7 @@ describe('diff-view', () => {
     );
 
     await waitFor(() => expect(mockApi.getDiff).toHaveBeenCalled());
+    await screen.findByText('Hi');
     expect(screen.queryByRole('alert')).toBeNull();
-    screen.getByText('Hi');
   });
 });

src/components/__tests__/diff-view.test.jsx

@@ -1,6 +1,6 @@
 /* eslint-env jest */
 
-import { render, screen, fireEvent, waitFor } from '@testing-library/react';
+import { render, screen, fireEvent, waitFor, act } from '@testing-library/react';
 import LoginPanel from '../login-form/login-form';
 import WebMonitoringDb from '../../services/web-monitoring-db';
 import { ApiContext } from '../api-context';
@@ -35,10 +35,14 @@ describe('login-form', () => {
     screen.getByRole('button', { name: 'Cancel' });
   });
 
-  it('Calls props.cancelLogin when the cancel button is clicked', () => {
+  it('Calls props.cancelLogin when the cancel button is clicked', async () => {
     const cancelLogin = jest.fn();
     render(<LoginPanel cancelLogin={cancelLogin} />);
-    fireEvent.click(screen.getByRole('button', { name: 'Cancel' }));
+
+    await act(() => {
+      fireEvent.click(screen.getByRole('button', { name: 'Cancel' }));
+    });
+
     expect(cancelLogin).toHaveBeenCalled();
   });
 
@@ -51,9 +55,11 @@ describe('login-form', () => {
         </ApiContext.Provider>
       );
 
-      fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
-      fireEvent.change(screen.getByLabelText(/password/i), { target: { value: 'password' } });
-      fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      await act(() => {
+        fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
+        fireEvent.change(screen.getByLabelText(/password/i), { target: { value: 'password' } });
+        fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      });
 
       expect(api.logIn).toHaveBeenCalledWith('aaa@aaa.aaa', 'password');
     });
@@ -67,9 +73,11 @@ describe('login-form', () => {
         </ApiContext.Provider>
       );
 
-      fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
-      fireEvent.change(screen.getByLabelText(/password/i), { target: { value: 'password' } });
-      fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      await act(() => {
+        fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
+        fireEvent.change(screen.getByLabelText(/password/i), { target: { value: 'password' } });
+        fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      });
 
       await waitFor(() => expect(onLogin).toHaveBeenCalledWith({ id: 5 }));
     });
@@ -83,9 +91,11 @@ describe('login-form', () => {
         </ApiContext.Provider>
       );
 
-      fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
-      fireEvent.change(screen.getByLabelText(/password/i), { target: { value: 'password' } });
-      fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      await act(() => {
+        fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
+        fireEvent.change(screen.getByLabelText(/password/i), { target: { value: 'password' } });
+        fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      });
 
       await screen.findByText('Login unsuccessful');
     });
@@ -98,8 +108,10 @@ describe('login-form', () => {
         </ApiContext.Provider>
       );
 
-      fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
-      fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      await act(() => {
+        fireEvent.change(screen.getByLabelText(/e-?mail/i), { target: { value: 'aaa@aaa.aaa' } });
+        fireEvent.click(screen.getByRole('button', { name: 'Log In' }));
+      });
 
       expect(api.logIn).not.toHaveBeenCalled();
       await screen.findByText('Please enter an e-mail and password.');

src/components/__tests__/login-form.test.jsx

@@ -1,5 +1,5 @@
 /* eslint-env jest */
-import { render, waitFor } from '@testing-library/react';
+import { render, waitFor, screen } from '@testing-library/react';
 import PageDetails from '../page-details/page-details';
 import simplePage from '../../__mocks__/simple-page.json';
 import { ApiContext } from '../api-context';
@@ -45,15 +45,15 @@ describe('page-details', () => {
     });
   };
 
-  it('can render', () => {
+  it('can render', async () => {
     const mockApi = createMockApi();
-    const { container } = render(
+    render(
       <ApiContext.Provider value={{ api: mockApi }}>
         <PageDetails match={match} />
       </ApiContext.Provider>
     );
 
-    expect(container).not.toBeEmptyDOMElement();
+    await screen.findByText(simplePage.title);
   });
 
   it('shows correct title', async () => {

src/components/__tests__/page-details.test.jsx

@@ -1,6 +1,6 @@
 /* eslint-env jest */
 
-import { fireEvent, render, screen } from '@testing-library/react';
+import { fireEvent, render, screen, act } from '@testing-library/react';
 import PageList from '../page-list/page-list';
 import simplePages from '../../__mocks__/simple-pages.json';
 
@@ -56,25 +56,29 @@ describe('page-list', () => {
     expect(screen.queryByText(/loading/i)).toBeNull();
   });
 
-  it('opens a new window when a user control clicks on a page row', () => {
+  it('opens a new window when a user control clicks on a page row', async () => {
     global.open = jest.fn();
     render(<PageList pages={simplePages} />);
 
     const page = simplePages[0];
-    const row = screen.getByRole('link', { name: page.url }).closest('tr');
-    fireEvent.click(row, { ctrlKey : true });
+    await act(() => {
+      const row = screen.getByRole('link', { name: page.url }).closest('tr');
+      fireEvent.click(row, { ctrlKey : true });
+    });
 
     expect(global.open.mock.calls[0][0]).toBe(`/page/${page.uuid}`);
     expect(global.open.mock.calls[0][1]).toBe('_blank');
   });
 
-  it('opens a new window when a user command clicks on a page row', () => {
+  it('opens a new window when a user command clicks on a page row', async () => {
     global.open = jest.fn();
     render(<PageList pages={simplePages} />);
 
     const page = simplePages[0];
-    const row = screen.getByRole('link', { name: page.url }).closest('tr');
-    fireEvent.click(row, { metaKey : true });
+    await act(() => {
+      const row = screen.getByRole('link', { name: page.url }).closest('tr');
+      fireEvent.click(row, { metaKey : true });
+    });
 
     expect(global.open.mock.calls.length).toBe(1);
   });