fixes #756: don't make the odata request when route is being changed

[sadiqkhoja]

Closes central#756

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

Added couple of tests, also manually verified that issue is fixed.

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

Root cause: when user clicks on another page link, query parameter changes, which causes odataFilter watcher in SubmissionList component to send another odata request.

If a route is being changed, hence component is being unmounted then that component should not make a new (update) request.

Another approach was to add null check for _abortController in resource.js:cancelRequest(), which felt like hacky to me. or If _store is being shared like in this case, maybe we can share the _abortController as well; I am not fully certain about its implications though.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Any page that have filters like Entities data and System audit should be tested as well for regressions.

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

NA

Before submitting this PR, please make sure you have:

• run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
• verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

[matthew-white]

I wanted to look into this, because I don't understand how this is happening given how watchers work. From the Vue docs:

Watchers declared synchronously inside setup() or <script setup> are bound to the owner component instance, and will be automatically stopped when the owner component is unmounted.

As you say, the navigation away from the submissions page means that the component for the page is unmounted. So why is a watcher on that page firing?

I added some logging to see what was going on:

useQueryRef()

 watch(router.currentRoute, ({ query: newQuery }, { query: oldQuery }) => {
+   console.log('route change: useQueryRef()');

src/router.js

import { ticking } from './util/debug';

// ...

router.afterEach(() => {
  console.log('route change: afterEach()');
  ticking(10);
}));

watchSync(router.currentRoute, () => {
  console.log('route change: watchSync()');
});

FormSubmissions

beforeUnmount() { console.log('FormSubmissions: beforeUnmount'); },
unmounted() { console.log('FormSubmissions': unmounted'); },
watch: {
  $route() { console.log('route change: FormSubmissions watcher'); }
},

SubmissionList

 watch: {
+   $route() { console.log('route change: SubmissionList watcher'); },

FormDraftTesting

 <script setup>
 ...
+console.log('FormDraftTesting: created');

route change: watchSync()
route change: afterEach()
route change: FormSubmissions watcher
route change: useQueryRef()
route change: SubmissionList watcher
route change: useQueryRef() x3
FormSubmissions: beforeUnmount
FormSubmissions: unmounted
tick x10
FormDraftTesting: created

What's noteworthy to me is the fact that "tick" isn't logged until after FormSubmissions is unmounted, because it confirms one aspect of how I thought the router worked: that the route component is unmounted in the same tick in which router.currentRoute is changed (i.e., it is unmounted synchronously). Yet even though the route component is about to be unmounted synchronously, the watcher in useQueryRef() reacts synchronously to the change to router.currentRoute. What's particularly strange is that the watcher in useQueryRef() isn't a synchronous watcher: we're using watch(), not watchSync(). The watcher is supposed to react to changes asynchronously, yet here it's doing so synchronously.

I think what must be happening is that when Vue goes to unmount a component, it gives the component's watchers one last chance to react to changes.

But just to throw out one last curiosity, if I call watch() with { flush: 'post' }, it isn't run before the component is unmounted. I feel like I want to check some of this the next time we update Vue. It just seems like odd behavior.

I'm glad to have looked into this though. Something similar came up with a watcher in another PR recently: #1024 (comment).

[matthew-white]

A totally separate curiosity is that I'm actually seeing different behavior from what's described in the issue. The issue describes an error message and the fact that no submissions are shown on the draft testing page. I'm not seeing that. What I am seeing is that the useQueryRef() watcher causes the submissions page to fire off an unneeded request for OData before going on to immediately abort the request. (You can see that in the Network tab of Chrome devtools.) I definitely think we should prevent that, but it also seems less serious to me than the page failing to render.

Looking at the error message in the issue, it's a little puzzling to me:

TypeError: Cannot read properties of null (reading 'abort')
    at Proxy.cancelRequest (resource.js:67:71)

That refers to this line:

central-frontend/src/request-data/resource.js

Line 75 in eea4324

cancelRequest() { if (this.awaitingResponse) this[_abortController].abort(); }

An error would be thrown here if this.awaitingResponse && this[_abortController] == null. However, that should be impossible. I'm very careful in the request() method to always set this.awaitingResponse and this[_abortController] in the same tick. The following is the only place where I set this[_abortController] to null, and you can see that this.awaitingResponse becomes false at the same time:

central-frontend/src/request-data/resource.js

Lines 231 to 240 in eea4324

	const cleanup = () => {
	// Each request is associated with its own AbortController. If another
	// request is sent for this same resource, then that will cancel this
	// request and immediately set this[_abortController] to the
	// AbortController associated with the new request. In that case, we will
	// let the new request complete this part of the cleanup.
	if (this[_abortController] === abortController) {
	this[_store].awaitingResponse = false;
	this[_abortController] = null;
	}

I think the error must have been due to the fact that FormSubmissions and FormDraftTesting were using the same requestData store for the odata local resource in SubmissionList, which they both render. In other words, FormSubmissions and FormDraftTesting each create a local resource, but because the two local resources have the same name (odata), they were using the same store under the hood. I actually changed how that worked very recently, in #1050. As of that PR, local resources are independent of one another and don't share the same store.

Before #1050 came in, I'm guessing that there was a sequence along these lines:

• The useQueryRef() watcher causes SubmissionList to send a final, futile request for OData before the component is unmounted.
• It's a local resource, so when SubmissionList is unmounted, it calls cancelRequest() on the resource. That will abort the request, which will cause the cleanup() function from above to be called. It's not called synchronously though, but instead only after the axios promise resolves (very soon).
• Apparently it's not soon enough though! When the new SubmissionList is mounted, it will send its own request for OData. If cleanup() hasn't been called yet, then this.awaitingResponse will be true even before the request is sent. That's an effect of the two local resources using the same store. Yet this[_abortController] is not persisted in the store, because it doesn't need to be reactive. That means that this[_abortController] will be null in the new local resource. The combination of this.awaitingResponse being true and this[_abortController] being null would lead to the error above.

TL;DR it's a good thing that local resources are now independent of each other. That prevents unexpected sequences like the one above and prevents errors like the one described in the issue. There's still something to fix though, which is that we should prevent the unnecessary OData request from getting sent.

[matthew-white]

I'm going to push some small changes. Before that, I wanted to leave some comments explaining a couple of my changes.

[matthew-white]

I think we should use the route path instead:

• In general, I try not to use route names to drive behavior. We need to do so in src/routes.js, but outside that, we almost never use route names.
• If the user navigates from the submissions page for one form to the submissions page for a different form (e.g., via a bookmark), then the route path will change, but the route name will not. But we still want to stop the watcher in that case.

[sadiqkhoja]

I agree, comparing path is better than name

[matthew-white]

This navigation would change the page and render a new route component. Given that, I think we should use load() instead of loadComponent(). That way, the route component can change.

[sadiqkhoja]

Thanks for digging deep into this. I agree with your assessments about watch is running synchronously and independent local resources have fixed the console error.