Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Vuex store to Pinia #1265

Merged
merged 5 commits into from
Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions web/src/main.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Import external packages
import Vue, { provide } from 'vue';
import { sync } from 'vuex-router-sync';
import Vue from 'vue';
import VueGtag from 'vue-gtag';
import VueSocialSharing from 'vue-social-sharing';

Expand All @@ -9,6 +8,7 @@ import * as Sentry from '@sentry/browser';
import * as Integrations from '@sentry/integrations';

// Import plugins first (order may matter)
import pinia from '@/plugins/pinia';
import vuetify from '@/plugins/vuetify';

// Import custom behavior
Expand All @@ -17,28 +17,22 @@ import '@/title';
// Import internal items
import App from '@/App.vue';
import router from '@/router';
import store from '@/store';

Sentry.init({
dsn: process.env.VUE_APP_SENTRY_DSN,
environment: process.env.VUE_APP_SENTRY_ENVIRONMENT,
integrations: [new Integrations.Vue({ Vue, logErrors: true })],
});

sync(store.original, router);

Vue.use(VueGtag, {
config: { id: 'UA-146135810-2' },
}, router);

Vue.use(VueSocialSharing);

new Vue({
setup() {
provide('store', store);
},
router,
render: (h) => h(App),
store: store.original,
pinia,
vuetify,
}).$mount('#app');
6 changes: 6 additions & 0 deletions web/src/plugins/pinia.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import Vue from 'vue';
import { createPinia, PiniaVuePlugin } from 'pinia';

Vue.use(PiniaVuePlugin);

export default createPinia();
152 changes: 0 additions & 152 deletions web/src/store/dandiset.ts

This file was deleted.

40 changes: 0 additions & 40 deletions web/src/store/index.ts

This file was deleted.

112 changes: 112 additions & 0 deletions web/src/stores/dandiset.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/* eslint-disable no-use-before-define */

import { defineStore } from 'pinia';

import axios from 'axios';
import RefParser from '@apidevtools/json-schema-ref-parser';

// eslint-disable-next-line import/no-cycle
import { dandiRest } from '@/rest';
import { User, Version } from '@/types';
import { draftVersion } from '@/utils/constants';

interface State {
dandiset: Version | null;
versions: Version[] | null,
loading: boolean,
owners: User[] | null,
schema: any,
}

export const useDandisetStore = defineStore('dandiset', {
state: (): State => ({
dandiset: null,
versions: null,
loading: false, // No mutation, as we don't want this mutated by the user
mvandenburgh marked this conversation as resolved.
Show resolved Hide resolved
owners: null,
schema: null,
}),
getters: {
version: (state) => (state.dandiset ? state.dandiset.version : draftVersion),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we apply TS types to these functions? It would make the list of getters that much more legible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike Vuex with direct-vuex, Pinia is fully typed, so the types of every function parameter/return value here are inferred automatically without needing to manually specify them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, maybe I'm misunderstanding what you're saying. Are you just asking for explicit types to be added? Like

Suggested change
version: (state) => (state.dandiset ? state.dandiset.version : draftVersion),
version: (state): string => (state.dandiset ? state.dandiset.version : draftVersion),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant the latter. It's not necessary to fill them all in now, but I think one of the major benefits of TypeScript is that the types are checked by the machine, but also legible to the human.

schemaVersion: (state) => state.schema?.properties.schemaVersion.default,
userCanModifyDandiset: (state) => {
const user = dandiRest?.user as User | null;

// published versions are never editable, and logged out users can never edit a dandiset
if (state.dandiset?.metadata?.version !== draftVersion || !user) {
return false;
}
// if they're an admin, they can edit any dandiset
if (user.admin) {
return true;
}
// otherwise check if they are an owner
return !!(state.owners?.find((owner) => owner.username === user.username));
},
},
actions: {
async uninitializeDandisets() {
this.dandiset = null;
this.versions = null;
this.owners = null;
this.loading = false;
},
async initializeDandisets({ identifier, version }: Record<string, string>) {
this.uninitializeDandisets();

// this can be done concurrently, don't await
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a TODO?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I believe it's just there to clarify why there's no await in front of the following method so someone doesn't see it and think it's a bug. It was there in the old vuex code, I just copied it over.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The confusion I had was between what you just said (explaining why the first statement has no await) and suggesting that someone should remove the following two awaits and make all three operations concurrent.

this.fetchDandisetVersions({ identifier });
await this.fetchDandiset({ identifier, version });
await this.fetchOwners(identifier);
},
async fetchDandisetVersions({ identifier }: Record<string, string>) {
this.loading = true;
const res = await dandiRest.versions(identifier);
if (res) {
const { results } = res;
this.versions = results || [];
}

this.loading = false;
},
async fetchDandiset({ identifier, version }: Record<string, string>) {
this.loading = true;
const sanitizedVersion = version || (await dandiRest.mostRecentVersion(identifier))?.version;

if (!sanitizedVersion) {
this.dandiset = null;
this.loading = false;
return;
}

try {
const data = await dandiRest.specificVersion(identifier, sanitizedVersion);
this.dandiset = data;
} catch (err) {
waxlamp marked this conversation as resolved.
Show resolved Hide resolved
this.dandiset = null;
}

this.loading = false;
},
async fetchSchema() {
const { schema_url: schemaUrl } = await dandiRest.info();
const res = await axios.get(schemaUrl);

if (res.status !== 200) {
throw new Error('Could not retrieve Dandiset Schema!');
}

const schema = await RefParser.dereference(res.data);

this.schema = schema;
},
async fetchOwners(identifier: string) {
this.loading = true;

const { data } = await dandiRest.owners(identifier);
this.owners = data;

this.loading = false;
},
},
});