Don't require local resources and getters to have globally unique names

src/components/async-route.vue

@@ -69,40 +69,15 @@ export default {
     propsAndAttrs() {
       // The main use of this.$attrs is to pass along event listeners to the
       // component.
-      return { ...this.props, ...this.$attrs };
+      return { ...this.props, ...this.$attrs, key: this.k };
     },
     componentNameAndKey() {
       return [this.componentName, this.k];
     }
   },
   watch: {
     componentNameAndKey([newComponentName], [oldComponentName]) {
-      if (newComponentName !== oldComponentName) {
-        this.load();
-      } else if (this.component != null) {
-        /*
-        If this.k has changed, then we need to re-render the component (unless
-        this.component == null, in which case there is no component to
-        re-render). We will cause a re-render by setting this.component to
-        `null` for a tick.
-
-        Previously, we used the `key` attribute to cause the component to
-        re-render. However, that results in the following lifecycle stages:
-
-          - `beforeUnmount` for the old component
-          - `setup` for the new component
-          - `unmounted` for the old component
-
-        Because we use `unmounted` hooks with requestData, we need `unmounted`
-        for the old component to happen before `setup` for the new component.
-        Otherwise, the new component might try to create a local resource with
-        the same name as one created by the old component, whose local resources
-        haven't been removed yet.
-        */
-        const { component } = this;
-        this.component = null;
-        this.$nextTick(() => { this.component = component; });
-      }
+      if (newComponentName !== oldComponentName) this.load();
     }
   },
   created() {

src/components/entity/conflict-summary.vue

@@ -109,10 +109,10 @@ const markAsResolved = () => {
     .then(({ data }) => {
       hideConfirm();
       alert.success(t('conflictResolved'));
-      entity.patch(() => {
-        entity.conflict = data.conflict;
-        entity.updatedAt = data.updatedAt;
-      });
+
+      entity.conflict = data.conflict;
+      entity.updatedAt = data.updatedAt;
+
       emit('resolve');
     })
     .catch(noop);

src/components/entity/show.vue

@@ -128,15 +128,14 @@ const afterUpdate = (updatedEntity) => {
   fetchActivityData();
   updateModal.hide();
   alert.success(i18n.t('alert.updateEntity'));
-  entity.patch(() => {
-    // entity.currentVersion will no longer have extended metadata, but we don't
-    // need it to.
-    entity.currentVersion = updatedEntity.currentVersion;
-    entity.updatedAt = updatedEntity.updatedAt;
-    // Update entity.conflict in case a conflict has been resolved by another
-    // user or in another tab.
-    entity.conflict = updatedEntity.conflict;
-  });
+
+  // entity.currentVersion will no longer have extended metadata, but we don't
+  // need it to.
+  entity.currentVersion = updatedEntity.currentVersion;
+  entity.updatedAt = updatedEntity.updatedAt;
+  // Update entity.conflict in case a conflict has been resolved by another
+  // user or in another tab.
+  entity.conflict = updatedEntity.conflict;
 };
 
 const deleteModal = modalData();

src/components/form-attachment/list.vue

@@ -337,17 +337,17 @@ export default {
           if (this.$route !== initialRoute) return;
           if (updates.length === this.uploadStatus.total)
             this.alert.success(this.$tcn('alert.success', updates.length));
-          this.attachments.patch(() => {
-            for (const [name, updatedAt] of updates) {
-              const attachment = this.attachments.get(name);
-              attachment.blobExists = true;
-              attachment.datasetExists = false;
-              attachment.exists = true;
-              attachment.updatedAt = updatedAt;
 
-              this.updatedAttachments.add(name);
-            }
-          });
+          for (const [name, updatedAt] of updates) {
+            const attachment = this.attachments.get(name);
+            attachment.blobExists = true;
+            attachment.datasetExists = false;
+            attachment.exists = true;
+            attachment.updatedAt = updatedAt;
+
+            this.updatedAttachments.add(name);
+          }
+
           this.uploadStatus = { total: 0, remaining: 0, current: null, progress: 0 };
         });
       this.plannedUploads = [];
@@ -367,11 +367,9 @@ export default {
         attachmentName: attachment.name
       }));
 
-      this.attachments.patch(() => {
-        attachment.datasetExists = true;
-        attachment.blobExists = false;
-        attachment.exists = true;
-      });
+      attachment.datasetExists = true;
+      attachment.blobExists = false;
+      attachment.exists = true;
     }
   }
 };

src/container.js

@@ -11,15 +11,13 @@ except according to the terms contained in the LICENSE file.
 */
 import axios from 'axios';
 import { Translation } from 'vue-i18n';
-import { createPinia } from 'pinia';
 
 import createAlert from './alert';
 import createCentralI18n from './i18n';
 import createCentralRouter from './router';
 import createUnsavedChanges from './unsaved-changes';
 import { $tcn } from './util/i18n';
 import { createRequestData } from './request-data';
-import { noop } from './util/util';
 
 const provide = [
   'alert',
@@ -29,8 +27,6 @@ const provide = [
   'logger'
 ];
 
-const piniaMock = { install: noop };
-
 export default ({
   // `router` must be a function that returns an object. The function will be
   // passed a partial container. It is also possible to create a container
@@ -47,7 +43,6 @@ export default ({
   logger = console
 } = {}) => {
   const container = {
-    pinia: process.env.NODE_ENV === 'development' ? createPinia() : piniaMock,
     i18n: i18n.global,
     alert,
     unsavedChanges,
@@ -64,7 +59,6 @@ export default ({
     // eslint-disable-next-line no-param-reassign
     app.config.globalProperties.$tcn = $tcn;
 
-    app.use(container.pinia);
     app.use(container.requestData);
     if (container.router != null) app.use(container.router);
 

src/request-data/index.js

@@ -9,51 +9,26 @@ https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
 including this file, may be copied, modified, propagated, or distributed
 except according to the terms contained in the LICENSE file.
 */
-import { computed, inject, onUnmounted, provide, shallowReactive } from 'vue';
-import { defineStore } from 'pinia';
+import { computed, inject, onUnmounted, provide } from 'vue';
 
 import createResources from './resources';
 import { createResource, resourceView } from './resource';
 
-const createState = () =>
-  shallowReactive({ data: null, awaitingResponse: false });
-const patchMock = (f) => { f(); };
-const defineResourceStore = false // eslint-disable-line no-constant-condition
-  ? (name) => defineStore(`requestData.${name}`, { state: createState })
-  : () => {
-    const store = createState();
-    store.$patch = patchMock;
-    return () => store;
-  };
-
 const composableKey = Symbol('requestData composable');
 
 export const createRequestData = (container) => {
-  // `resources` holds both app-wide resources and local resources.
-  const resources = new Map();
-  /* `stores` holds all the Pinia stores that have ever been created for a
-  resource. Even after a local resource is removed from `resources` (when the
-  component is unmounted), the resource's underlying store is not removed from
-  `stores`. That's because we will reuse the store if another local resource is
-  created with the same name. Doing so minimizes the number of stores shown in
-  Vue Devtools. */
-  const stores = new Map();
-
   // Create app-wide resources.
   const requestData = { resources: new Set() };
-  const { pinia } = container;
   createResources(container, (name, setup) => {
-    const useStore = defineResourceStore(name);
-    stores.set(name, useStore);
-    const resource = createResource(container, name, useStore(pinia), setup);
-    resources.set(name, resource);
+    if (name in requestData) throw new Error(`the name ${name} is in use`);
+    const resource = createResource(container, name, setup);
     requestData[name] = resource;
     requestData.resources.add(resource);
     return resource;
   });
 
   // Checking the collective state of multiple resources
-  requestData.resourceStates = (resources) => ({ // eslint-disable-line no-shadow
+  requestData.resourceStates = (resources) => ({
     // Returns `true` if a group of resources is being initially loaded (not
     // refreshed). Returns `false` if there is a resource that is not loading
     // and also does not have data, because presumably its request resulted in
@@ -64,83 +39,55 @@ export const createRequestData = (container) => {
     dataExists: computed(() => resources.every(resource => resource.dataExists))
   });
 
-  // Local resources
+  // Add a mechanism to create local resources. Local resources do not have to
+  // have names that are globally unique: different local resources can have the
+  // same name at the same time. The localResources object keys by name, but it
+  // is only used in testing and only in situations where that should be OK.
+  const localResources = Object.create(null);
   const createLocalResource = (name, setup) => {
-    // Even local resources cannot share a name at the same time. That's because
-    // the name is used for the id of the store.
-    if (resources.has(name)) throw new Error(`the name ${name} is in use`);
-    if (!stores.has(name)) {
-      const useStore = defineResourceStore(name);
-      stores.set(name, useStore);
-    }
-    const useStore = stores.get(name);
-    const resource = createResource(container, name, useStore(), setup);
-    resources.set(name, resource);
+    const resource = createResource(container, name, setup);
     // We provide the local resource; only descendant components will be able to
     // inject it.
     provide(`requestData.localResources.${name}`, resource);
+    localResources[name] = resource;
 
     onUnmounted(() => {
-      /*
-      Resetting the resource will reset the state of the store, allowing the
-      store to be reused if a local resource is created later with the same
-      name.
-
-      It's because we reset the resource that we use an `unmounted` hook rather
-      than a beforeUnmount hook. If we used a beforeUnmount hook, then if the
-      component used watchSyncEffect() with the resource, the watchSyncEffect()
-      callback would be run after the resource was reset. That seems surprising,
-      and accounting for that possibility could add complexity to the
-      watchSyncEffect() callback and the component. (That said, using an
-      `unmounted` hook does end up adding complexity to AsyncRoute.)
-      */
-      resource.reset();
-      resources.delete(name);
+      // The resource is no longer needed, so we cancel any request still in
+      // progress. We use onUnmounted() rather than onBeforeUnmount() in case
+      // the component is watching resource.awaitingResponse synchronously for
+      // some reason. We don't want to trigger a component watcher while the
+      // component is being unmounted.
+      resource.cancelRequest();
+
+      // Remove `resource` from localResources unless it has already been
+      // replaced.
+      if (localResources[name] === resource) delete localResources[name];
     });
 
     return resource;
   };
-  if (process.env.NODE_ENV === 'test') {
-    // Normally, requestData only provides access to app-wide resources.
-    // However, in testing, we sometimes need access outside components to local
-    // resources.
-    requestData.localResources = new Proxy({}, {
-      get: (_, prop) => {
-        const resource = resources.get(prop);
-        return resource != null && !requestData.resources.has(resource)
-          ? resource
-          : undefined;
-      }
-    });
-  }
+  // Normally, requestData only provides access to app-wide resources. However,
+  // we allow tests to access local resources in order to make assertions about
+  // them.
+  if (process.env.NODE_ENV === 'test')
+    requestData.localResources = localResources;
 
   // Getters allow you to create a computed ref in one component, then access it
   // from useRequestData() in a descendant component. Getters are only really
   // needed for computed refs that reference multiple resources.
-  const getters = new Map();
+  const getters = Object.create(null);
   const createGetter = (name, ref) => {
-    // Unlike with resources, there's no real problem with different getters
-    // using the same name. Supporting that case would just make managing
-    // `getters` harder.
-    if (getters.has(name)) throw new Error(`the name ${name} is in use`);
-    getters.set(name, ref);
     provide(`requestData.getters.${name}`, ref);
-    onUnmounted(() => { getters.delete(name); });
+    onUnmounted(() => { if (getters[name] === ref) delete getters[name]; });
     return ref;
   };
 
   // useRequestData()
   const getResource = (name) => {
-    const resource = resources.get(name);
-    if (resource == null) return null;
-    // If the resource is a local resource, check whether it was created by an
-    // ancestor component.
-    if (!requestData.resources.has(resource) &&
-      inject(`requestData.localResources.${name}`) == null)
-      return null;
-    const useStore = stores.get(name);
-    useStore();
-    return resource;
+    const localResource = inject(`requestData.localResources.${name}`, null);
+    if (localResource != null) return localResource;
+    const value = requestData[name];
+    return requestData.resources.has(value) ? value : null;
   };
   const composable = new Proxy(
     {
@@ -153,29 +100,21 @@ export const createRequestData = (container) => {
       createGetter
     },
     {
-      get: (target, prop) => {
-        const resource = getResource(prop);
-        if (resource != null) return resource;
-
-        if (getters.has(prop)) {
-          const getter = inject(`requestData.getters.${prop}`);
-          if (getter != null) return getter;
-        }
-
-        return requestData[prop] != null ? requestData[prop] : target[prop];
-      }
+      get: (target, prop) => getResource(prop) ??
+        inject(`requestData.getters.${prop}`, null) ??
+        requestData[prop] ??
+        target[prop]
     }
   );
 
   requestData.install = (app) => {
     app.provide(composableKey, composable);
 
-    // Provide local resources and getters for testing.
-    for (const [name, resource] of resources.entries()) {
-      if (!requestData.resources.has(resource))
-        app.provide(`requestData.localResources.${name}`, resource);
-    }
-    for (const [name, getter] of getters.entries())
+    // Provide local resources and getters to a component being mounted in
+    // testing.
+    for (const [name, resource] of Object.entries(localResources))
+      app.provide(`requestData.localResources.${name}`, resource);
+    for (const [name, getter] of Object.entries(getters))
       app.provide(`requestData.getters.${name}`, getter);
   };