Reload page after vite:preloadError

[matthew-white]

Closes getodk/central#2073.

What has been done to verify that this works as intended?

Mostly I just wrote a new test.

I didn't try this, but one thing I could do is try to trigger a real vite:preloadError, e.g., by building files locally, changing some code, then taking an action that attempts to load the changed asset. That wouldn't work with HMR, but we could use npm run dev:build to try to see that.

Another thing we'll do is continue to watch whether this error appears in Sentry.

Why is this the best possible solution? Were any other approaches considered?

The main thing I want to make note of is that I put this code in a new composable, useVersionMonitor(). The /version.txt check lives in the App component, but I feel like there's already too much going on in that component; I didn't want to add to it. Instead, I plan to move the /version.txt check from the App component into the new useVersionMonitor() composable. The /version.txt check and the vite:preloadError event listener are two different ways to monitor for a change to the version of Central deployed.

How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?

I think the risk of regression is low, as it's entirely new code (just additions). In the issue, we discussed the potential user impact.

[matthew-white]

Adding comments to facilitate async code review.

[matthew-white]

As mentioned in the PR description, I plan to move the /version.txt check into this file soon. I like the idea of centralizing that logic here and simplifying the App component.

[matthew-white]

In production, location is the same as window.location. In testing, it can be mocked. It is a property of the container object and is provided via container: see src/container.js.

[matthew-white]

Unfortunately, it looks like e2e tests are consistently failing with this change. You can see that in CI above and also in getodk/web-forms#884, which I filed before closing.

I can think of two options for how to proceed:

1. Go back to the idea of showing a toast instead of automatically reloading the page.
2. Somehow detect whether it's a CI/test/e2e environment and not reload if so.
   • Maybe we could check for localhost in the URL?
   • Or maybe we add a new property to src/config.js or client-config.json to indicate whether we're in CI? Not exactly sure what that plumbing would look like, but basically adding some sort of configuration to drive whether the reload actually happens.

Why is vite:preloadError being triggered in e2e tests? I feel like I've seen related discussion about that, but I don't think I understand the cause of it.

[matthew-white]

Another idea: we could wait a few seconds before adding the event listener. Definitely feels like a workaround, but it'd fix the e2e tests without having to add configuration.

[matthew-white]

we could wait a few seconds before adding the event listener.

I've implemented logic around those lines, and e2e tests now pass. I don't wait to add the event listener itself, but the event listener will skip the automatic reload within the first seconds of app startup.

@latin-panda, I think this PR is fully ready for review now!

[latin-panda]

Thanks @matthew-white. It's the first item on my list to review tomorrow!

[latin-panda]

Thank you! Just one suggestion below. Approving to unblock

[latin-panda]

Can we add event.preventDefault() here? Without it, the error still throws even though the page reloads.

Suggested change

	useEventListener(window, 'vite:preloadError', () => {
	// Don't reload right after app startup, as that can break e2e tests.
	if (Date.now() - start >= 30000) location.reload();
	});
	useEventListener(window, 'vite:preloadError', (event) => {
	// Don't reload right after app startup, as that can break e2e tests.
	if (Date.now() - start >= 30000) {
	event.preventDefault();
	location.reload();
	}
	});

[matthew-white]

Done in e497d5e

[matthew-white]

I plan to move the /version.txt check from the App component into the new useVersionMonitor() composable.

I've filed a follow-up issue about this: getodk/central#2022.