Upgrade React 18 #3793

adghayes · 2023-08-06T02:18:30Z

Overview

Closes #1660. Closes #2798.

Demo Video or Screenshot

Testing Plan

Checklist

I have added logging where appropriate to any new user actions, system updates such as file reads or storage writes, or errors introduced.

I have added a screenshot and/or video to this PR to demo the change
I have added the "user_facing_change" label to this PR to automate an announcement in #machine-product-updates

apps/admin/frontend/package.json

package.json

libs/ui/src/export_logs_modal.test.tsx

adghayes · 2023-08-06T02:33:35Z

apps/scan/frontend/src/screens/poll_worker_screen.test.tsx

@@ -233,7 +232,6 @@ describe('transitions from polls paused', () => {
    apiMock.expectSetPollsState('polls_open');
    apiMock.expectGetConfig({ pollsState: 'polls_open' });
    userEvent.click(screen.getByText('Yes, Resume Voting'));
-    await screen.findByText('Resuming Voting…');


This is among a few cases where loading states are no longer visible in tests without setting up some sort of deferred promise, and it just didn't seem worth the trouble.

Do we have other coverage of this? I think one of the main responsibilities of the frontend tests should be covering the various states we expect to see.

I get that it feels tedious when it takes a lot of work. Maybe that just means we need to make it easier to set up

Actually I was wrong, I think I was just avoiding an act warning here. Added back the assertions, wrapped. a0b04d0

apps/admin/frontend/src/screens/print_test_deck_screen.tsx

adghayes · 2023-08-06T02:44:36Z

apps/mark/frontend/src/pages/accessible_controller_diagnostic_screen.test.tsx

  });

-  it('yields a success result when all steps are completed', async () => {
+  it.skip('yields a success result when all steps are completed', async () => {


The two tests for the successful diagnostics flow are failing. In the diagnostics flow, you press a button to play the sound, and then advance once the sound is played. You cannot confirm the test passed until the sound is played. So hasSoundPlayed is state of that component. See the useEffect here.

In testing, however, the state change setHasAudioPlayed never triggers the useEffect to run again so, when the {Enter} button is pressed, the test doesn't advance to passed. I tested in the app itself, and this works, so it seems specific to the testing environment. If you add some element of the render that is dependent on hasPlayedAudio, that forces a re-render, which does run the useEffect. But state changes should still trigger hooks to re-run even if there's no change on the page? I am confused by this - not sure if it's a react issue, an RTL issue, or expected behavior.

In any case, I can fix this in one of two ways:

Changing the screen to display something differently after audio has played. Maybe the "Sound is Not Working" button should be disabled until the sound has played? Or maybe we should have a check?

Explicitly calling rerender in the tests that this affects after the sound plays.

Have you confirmed that there is no re-render happening? Or is it just happening after we try to press {Enter}? I'm happy to look at this one closer with you.

Yeah, I have confirmed no re-render is happening, before or after {Enter}. Below is a summarized console output from adding console.log in a bunch of places. The render function never re-runs.

I would appreciate it if you looked into this one with me. My read of it, that state changes aren't propagating because they don't affect the DOM, is troubling.

Let me know if you want to play with it or want me to try something else specific.

console.log run render AccessibleControllerSoundDiagnostic run useEffect to set/reset listener trigger {ArrowRight} in test, to play sound keydown listener runs {ArrowRight} hasPlayedAudio is false listener sets hasPlayedAudio to true trigger {Enter} in test keydown listener receives {Enter} hasPlayedAudio is false test passed callback not triggered

My take after debugging this a bit is that the test has no way to wait for the re-render to happen since there's nothing changed in the DOM.

I think it's reasonable to add something to the DOM to help the test out (maybe something hidden if we can't think of a reasonable change to make on screen).

My other idea is that we could setHasPlayedAudio(true) right before actually playing the audio, which would result in a fairly similar UX.

if (event.key === 'ArrowRight') { const wasMuted = screenReader.isMuted(); screenReader.unmute(); + setHasPlayedAudio(true); await screenReader.speak( 'Press the select button to confirm sound is working.' ); screenReader.toggleMuted(wasMuted); - setHasPlayedAudio(true); }

adghayes · 2023-08-06T02:53:50Z

apps/mark/frontend/src/app_quit_on_idle.test.tsx

 });

-test('Voter idle timeout', async () => {
+test.skip('Voter idle timeout', async () => {


This is the thorniest test failure of the them all. It appears that once the idle timeout passes, this useEffect in the shared IdlePage component runs many, many times:

vxsuite/libs/mark-flow-ui/src/pages/idle_page.tsx

Lines 37 to 42 in 3ee2494

useEffect(() => {

if (countdown === 0 && !isLoading) {

setIsLoading(true);

onCountdownEnd();

}

}, [countdown, isLoading, onCountdownEnd]);

Setting isLoading to true should prevent the effect from re-running, but for some reason that state change doesn't propagate fast enough.

I tried this out in the app, in kiosk-browser, and found that it worked as expected. But I also noticed that the tree was re-rendering a ton, and traced it to this callback:

vxsuite/apps/mark/frontend/src/app_root.tsx

Lines 520 to 526 in 3ee2494

const endVoterSession = useCallback(async () => {

try {

await endCardlessVoterSessionMutation.mutateAsync();

} catch {

// Handled by default query client error handling

}

}, [endCardlessVoterSessionMutation]);

This is similar to we discussed here: #3191 (comment). Mutations returned from useMutation actually change on every render. And for some reason the app is re-rendering a lot. What I think is happening (due to React 18 concurrency and batching) is that the mutation within the IdlePage useEffect is triggering a circular issue where the mutation changes, the dependencies change, and the useEffect runs again before batched state changes take effect, so isLoading is still false.

One solution may be to have a ref for isLoading. In a subsequent PR, I fix by removing the mutation as a dependency of the callback, instead using the mutate function of the mutation.

Thoughts?

We could move the logic of that effect into the useInterval.

I also think we need to remove the mutation object from the dependency array like you mentioned. We should never be doing that, it's a footgun.

Excellent suggestion! That works. Fixed and pushed.

I created a ticket for the fix / lint rule for Mutation dependencies: #3794

adghayes · 2023-08-06T02:58:32Z

apps/mark/frontend/src/pages/idle_page.tsx

+  const reset = useCallback(() => {
+    endCardlessVoterSessionMutate(undefined, {
+      onSuccess: () => resetBallot(),
+    });
+  }, [endCardlessVoterSessionMutate, resetBallot]);


The mutate method has a consistent identity so doesn't cause runaway useEffects like were apparently botching that test.

I think we're still using mutations as dependencies in a lot of places. Annoyingly, the lint rule doesn't let you specify properties in the dependency array, some discussion here: facebook/react#16265. That lint rule is otherwise very helpful, so we don't want to disable it often. Which is a shame, because I think passing someMutation.mutate to the dependency array is more idiomatic than what I do above.

Thoughts?

How would you access the .mutate method inside the useCallback without also depending on the mutation object as well? Wouldn't the callback function close over the mutation object?

The callback function would close over the mutation object but, since the identify of .mutate is consistent, even if the mutation object we're accessing is old the .mutate reference should still be correct?

But we can't allow such a think within the react-hooks lint rule, unless there's a way to make programmatic carve-outs to rules (like post-processing to ignore certain cases). I assume that such a thing is possible within eslint, but maybe annoying.

Ah yes, I see how that would work. I don't mind the bit of extra boilerplate to pull it out into a separate variable. A little annoying, but that's ok.

jonahkagan

Awesome work! Excited to get this in.

jonahkagan · 2023-08-07T19:02:57Z

libs/ui/src/hooks/use_devices.test.ts

@@ -120,13 +120,13 @@ test('can connect printer as expected', async () => {
  );

  // Prevent `act` warning.
-  await waitForNextUpdate();


Is there still an act warning to prevent here?

jonahkagan · 2023-08-07T19:04:17Z

libs/ui/src/hooks/use_devices.test.ts

    useDevices({ hardware, logger })
  );
  expect(result.current).toEqual(emptyDevices);

-  await waitForNextUpdate();


Does waitForNextUpdate not work anymore?

It doesn't exist on the renderHook API added into mainline RTL. In the unmount case, I think we're supposed to use unmount. In the waiting on a status case, I think we're just supposed to use waitFor as is the pattern elsewhere.

Weird that they got rid of that. Oh well

jonahkagan · 2023-08-07T19:06:55Z

libs/ui/src/print_element.test.tsx

+    await suppressingConsoleOutput(async () => {
+      await printElement(simpleElement, fakeOptions);
+      expect(printer.print).toHaveBeenCalledTimes(1);
+      expect(printer.print).toHaveBeenLastCalledWith(
+        expect.objectContaining(fakeOptions)
+      );
    });


Happy to investigate this more with you or just leave as is

jonahkagan · 2023-08-07T19:09:02Z

apps/central-scan/frontend/src/components/delete_batch_modal.tsx

-          )}
+          {deleteBatchMutation.error ? (
+            <P color="danger">{deleteBatchMutation.error as string}</P>
+          ) : null}


Why this change?

The original, with @types upgrades, now throws lint error:

src/components/delete_batch_modal.tsx:30:10 - error TS2746: This JSX tag's 'children' prop expects a single child of type 'ReactNode', but multiple children were provided. 30 <React.Fragment> ~~~~~~~~~~~~~~

That doesn't make sense to me, because I thought the whole point of a fragment was that it could have multiple children. But if you change it to:

{deleteBatchMutation.error ? ( <P color="danger">{`${deleteBatchMutation.error}`}</P> ) : null}

Then it passes, and is also valid and effectively the same. The as string part can be removed. f0ac108.

I think the problem here is that deleteBatchMutation.error has type unknown. A few ideas for working around this...

Add a type assertion for that value:

{deleteBatchMutation.error as Error | undefined && <P color="danger">{`${deleteBatchMutation.error}`}</P>}

Plug in the Error type in the mutation:

return useMutation<void, Error, { batchId: string }>(apiClient.deleteBatch);

The problem with this is that TS doesn't support partial inference of type parameters, so you have to explicitly list all of them.

jonahkagan · 2023-08-07T19:13:18Z

apps/scan/frontend/src/app.test.tsx

  apiMock.mockApiClient.configureFromBallotPackageOnUsbDrive
    .expectCallWith()
-    .resolves(ok());
+    .returns(configurePromise);


If this ends up being a common pattern we might build it into the API mock. E.g. .resolves(value, { deferred: true }) could return a deferred

That could be neat! It's a bit unintuitive to switch from resolves(value) to returns(promise), so a resolves API would be easier to use.

jonahkagan · 2023-08-07T19:23:32Z

apps/mark/frontend/src/app_end_to_end.test.tsx

Heroic update to this file! Love seeing all the advanceTimersAndPromises go away. Would love to kill that function someday

jonahkagan · 2023-08-07T19:26:18Z

apps/mark/frontend/src/app_quit_on_idle.test.tsx

 });

-test('Voter idle timeout', async () => {
+test.skip('Voter idle timeout', async () => {


We could move the logic of that effect into the useInterval.

I also think we need to remove the mutation object from the dependency array like you mentioned. We should never be doing that, it's a footgun.

jonahkagan · 2023-08-07T19:28:43Z

apps/mark/frontend/src/app_setup_errors.test.tsx


    // Remove charger and reduce battery level slightly
    act(() => {
      hardware.setBatteryDischarging(true);
      hardware.setBatteryLevel(0.6);
    });
-    await advanceTimersAndPromises(BATTERY_POLLING_INTERVAL / 1000);


Does removing these significantly slow down the test overall? I think it's good to use fake timers and advance them when we have any sort of significant polling delay.

Nope, it has no impact on the speed of the test. Just confirmed. This test file uses fake timers and the waitFor and findBy... APIs are also advancing fake timers between each of their checks.

jonahkagan · 2023-08-07T19:39:45Z

apps/mark/frontend/src/pages/accessible_controller_diagnostic_screen.test.tsx

  });

-  it('yields a success result when all steps are completed', async () => {
+  it.skip('yields a success result when all steps are completed', async () => {


Have you confirmed that there is no re-render happening? Or is it just happening after we try to press {Enter}? I'm happy to look at this one closer with you.

jonahkagan · 2023-08-07T19:42:49Z

apps/mark/frontend/src/pages/idle_page.tsx

+  const reset = useCallback(() => {
+    endCardlessVoterSessionMutate(undefined, {
+      onSuccess: () => resetBallot(),
+    });
+  }, [endCardlessVoterSessionMutate, resetBallot]);


How would you access the .mutate method inside the useCallback without also depending on the mutation object as well? Wouldn't the callback function close over the mutation object?

This reverts commit 2905b3b.

adghayes added 10 commits August 5, 2023 11:39

package upgrades & linting overrides

6c9c7ab

libs/ui

8d0610a

libs/mark-flow-ui

8cb8f3e

apps/central-scan

5182d7c

apps/scan

937ccd5

apps/design

652149a

apps/admin

38b7441

apps/mark, skipping 3 tests

f949ac9

apps/mark-scan, skipping 3 tests

6677fe6

possible fix for idle page test

2905b3b

adghayes commented Aug 6, 2023

View reviewed changes

adghayes marked this pull request as ready for review August 7, 2023 16:30

adghayes requested review from jonahkagan, a team and eventualbuddha as code owners August 7, 2023 16:30

adghayes requested review from carolinemodic and removed request for a team August 7, 2023 16:30

jonahkagan approved these changes Aug 7, 2023

View reviewed changes

adghayes added 5 commits August 7, 2023 23:52

add back in testing for transition states in poll worker screen

a0b04d0

Revert "possible fix for idle page test"

082a8b6

This reverts commit 2905b3b.

move logic into useInterval in idle page per @jonahkagan's suggestion

f28087b

reverse typecasting

f0ac108

apply accessible controller diagnostic fix

5dbfe82

adghayes merged commit 2539d94 into main Aug 9, 2023
52 checks passed

adghayes deleted the drew/upgrade-react-1000 branch August 9, 2023 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade React 18 #3793

Upgrade React 18 #3793

adghayes commented Aug 6, 2023

adghayes Aug 6, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

adghayes Aug 6, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

jonahkagan Aug 8, 2023

adghayes Aug 6, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

jonahkagan Aug 8, 2023

adghayes Aug 6, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023 •

edited

jonahkagan Aug 8, 2023

jonahkagan left a comment

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

jonahkagan Aug 8, 2023

jonahkagan Aug 7, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023 •

edited

jonahkagan Aug 8, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

jonahkagan Aug 7, 2023

jonahkagan Aug 7, 2023

adghayes Aug 8, 2023

jonahkagan Aug 7, 2023

jonahkagan Aug 7, 2023

	useEffect(() => {
	if (countdown === 0 && !isLoading) {
	setIsLoading(true);
	onCountdownEnd();
	}
	}, [countdown, isLoading, onCountdownEnd]);

	const endVoterSession = useCallback(async () => {
	try {
	await endCardlessVoterSessionMutation.mutateAsync();
	} catch {
	// Handled by default query client error handling
	}
	}, [endCardlessVoterSessionMutation]);

Upgrade React 18 #3793

Upgrade React 18 #3793

Conversation

adghayes commented Aug 6, 2023

Overview

Demo Video or Screenshot

Testing Plan

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adghayes Aug 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahkagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adghayes Aug 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adghayes Aug 8, 2023 •

edited

adghayes Aug 8, 2023 •

edited