diff --git a/src/components/async-route.vue b/src/components/async-route.vue index 1d7ab7f98..5dd6f4b7c 100644 --- a/src/components/async-route.vue +++ b/src/components/async-route.vue @@ -69,7 +69,7 @@ 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]; @@ -77,32 +77,7 @@ export default { }, 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() { diff --git a/src/components/entity/conflict-summary.vue b/src/components/entity/conflict-summary.vue index d05c9942a..b145dd859 100644 --- a/src/components/entity/conflict-summary.vue +++ b/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); diff --git a/src/components/entity/show.vue b/src/components/entity/show.vue index ece878af1..48bfeaa8d 100644 --- a/src/components/entity/show.vue +++ b/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(); diff --git a/src/components/form-attachment/list.vue b/src/components/form-attachment/list.vue index 7f83c3ef9..507c55dee 100644 --- a/src/components/form-attachment/list.vue +++ b/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; } } }; diff --git a/src/container.js b/src/container.js index 6c3c3f5db..d0f9e6bc3 100644 --- a/src/container.js +++ b/src/container.js @@ -11,7 +11,6 @@ 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'; @@ -19,7 +18,6 @@ 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); diff --git a/src/request-data/index.js b/src/request-data/index.js index c299d8bf0..43456a63f 100644 --- a/src/request-data/index.js +++ b/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); }; diff --git a/src/request-data/resource.js b/src/request-data/resource.js index fcb37ccf9..bbed6234e 100644 --- a/src/request-data/resource.js +++ b/src/request-data/resource.js @@ -9,7 +9,7 @@ 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, isRef, readonly, toRef } from 'vue'; +import { computed, isRef, readonly, shallowReactive, toRef } from 'vue'; import { isProblem, logAxiosError, requestAlertMessage, withAuth, withHttpMethods } from '../util/request'; import { noop } from '../util/util'; @@ -40,21 +40,13 @@ class BaseResource { dataExists: computed(() => this.dataExists) }; } - - patch(data) { - this[_store].$patch(() => { - if (typeof data === 'function') - data(this.data); - else - Object.assign(this.data, data); - }); - } } const _container = Symbol('container'); const _abortController = Symbol('abortController'); class Resource extends BaseResource { - constructor(container, name, store) { + constructor(container, name) { + const store = shallowReactive({ data: null, awaitingResponse: false }); super(name, store); this[_container] = container; this[_abortController] = null; @@ -67,12 +59,8 @@ class Resource extends BaseResource { cancelRequest() { if (this.awaitingResponse) this[_abortController].abort(); } reset() { - if (this.dataExists || this.awaitingResponse) { - this[_store].$patch(() => { - this.data = null; - this.cancelRequest(); - }); - } + this.data = null; + this.cancelRequest(); } transformResponse(response) { return response.data; } @@ -272,7 +260,7 @@ class Resource extends BaseResource { this.setFromResponse(response); } else { if (!this.dataExists) throw new Error('data does not exist'); - this[_store].$patch(() => { patch(response, this); }); + patch(response, this); } }); } @@ -309,8 +297,8 @@ const proxyHandler = { /* eslint-enable no-param-reassign */ }; -export const createResource = (container, name, store, setup = undefined) => { - const resource = new Resource(container, name, store); +export const createResource = (container, name, setup = undefined) => { + const resource = new Resource(container, name); const proxy = new Proxy(resource, proxyHandler); if (setup != null) { diff --git a/src/request-data/user.js b/src/request-data/user.js index 29cdc4d5a..19ce5aa41 100644 --- a/src/request-data/user.js +++ b/src/request-data/user.js @@ -23,7 +23,7 @@ export default () => { watchSyncEffect(() => { // currentUser won't have data immediately after logout. if (user.dataExists && currentUser.dataExists && user.id === currentUser.id) - currentUser.patch(user.data); + Object.assign(currentUser.data, user.data); }); return user; };