Create a proof-of-concept for Pinia migration #906

dhruvkb · 2022-02-18T15:08:10Z

Migration to Pinia

Feel free to respond to this PR/RFC/proposal with

additional migration PRs (or commits)
comments suggesting changes to the approach

Goals

To completely eliminate all uses of Vuex, gradually replacing them with Pinia
To do the upgrade in a gradual way, entering a transition period where Vuex and Pinia coexist

Side-goals

Removal of the verbosity introduced by module and action constants
To create helpers, utils and patterns for testing store usage consistently in unit tests
Abandon the usage of the controversial useStore helper in favour of modular useX from Pinia stores

Steps

Install Pinia and test compatibility (this PR)
Replace Vuex stores with their Pinia counterparts

state -> state
getters -> getters
mutations and actions -> actions

In this stage we must determine if we must use mutations as actions. Pinia allows changing the state directly in the components (which was frowned upon by Vuex) so we could do away with mutations in theory. But having them and following the Vuex principle makes transition smoother and keeps state changes limited to one file instead of scattered around the codebase.

Update tests, tests involving changes to the Vuex store now need to be updated to work with Pinia.

beforeEach
```
setActivePinia(createPinia())
```

in the test case

const xyzStore = useXyz()
expect(xyzStore.state).to // ...

Remove unused constants from

~/constants/mutation-types.js
~/constants/action-types.js
~/constants/store-modules.js

-1. In the end, with no Vuex stores remaining, Vuex and all associated dependencies will be removed. The store/index.js file can also be removed after the complete migration to Pinia. The disableVuex: false flag should also be removed.

Conventions

Nomenclature

In the transition, we name stores after their Vuex counterparts.
- active.js → activeStore / useActive
State is unchanged, except that it is now an arrow function that returns the dictionary.
Mutations become the camelCasedVersions of their constant names
- SET_ACTIVE_MEDIA_ITEM → setActiveMediaItem

Type hinting

In the interest of a quick migration, we can continue to use the existing type definitions for the state and the various mutations/actions from the types.d.ts file. Some definitions in the file may be out of date (as was the case for ActiveMediaState), so a cursory glance to check that would be useful.

Procedure

To minimise disruption, each store will be changed using a lock-based approach where the developer takes a lock on the store using a notification to the group, makes a PR updating a single store and its usages and this lock is released after the PR is merged.

This prevents PRs from diverging too much and allows a seamless transition for all involved.

Fixes

Addresses #756 by @sarayourfriend
Addresses #831 by @obulat

PR description

This PR

sets up Pinia alongside Vuex
updates the smallest store, active, to Pinia
updates associated tests

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

sarayourfriend · 2022-02-18T15:15:14Z

Please note that the ATTRIBUTION store is being removed in #905

dhruvkb · 2022-02-18T15:16:35Z

Oh yeah, this was just a test to see if things work and if this is the right direction to proceed in, before we start migrating some of the more massive modules.

zackkrida · 2022-02-18T20:28:54Z

@dhruvkb this looks excellent, great initial proof of concept. I'd love to see this followed up with a Pina RFC next week to outline further next steps in more detail.

src/store/attribution.js

sarayourfriend · 2022-02-21T11:47:13Z

-1. In the end, with no Vuex stores remaining, Vuex and all associated dependencies will be removed. The store/index.js file can also be removed after the complete migration to Pinia.

We should also make a note to remove the disableVuex: false flag from the pinia configuration.

sarayourfriend

I'd love to see adding a requirement be TypeScript types for each re-written store. To include this we'd also have to add each re-written store to the tsconfig.json includes list.

This is great though!

In this stage we must determine if we must use mutations as actions. Pinia allows changing the state directly in the components (which was frowned upon by Vuex) so we could do away with mutations in theory. But having them and following the Vuex principle makes transition smoother and keeps state changes limited to one file instead of scattered around the codebase.

I'd be nice to stick mutations in the Pinia store, if only because it'll make them significantly easier to test without having to rely on triggering them via some component interaction.

in the test case

We can really just call the useX store composable inside the test function like that? Vue won't complain about using composables outside of a component context?

In your time researching, did you come across any opinions about whether to use the object or function callback syntax for defining a store? I strongly prefer the function callback syntax because I feel like it more closely mirrors the way state is created in setup (in fact, it's exactly the same) and it also makes having private helper functions possible (compared to the object syntax where all the functions on the definition object will be public). It also eliminates this usage which I've generally found to be a good thing for comprehensibility in JavaScript generally.

src/layouts/default.vue

src/store/active.js

src/components/VAudioTrack/VAudioTrack.vue

# Conflicts: # src/locales/po-files/openverse.pot

nuxt.config.ts

zackkrida

lgtm. My only remaining question is if we want to use the useNav naming convention or be extra redundant and clear with useNavStore, but that's a very minor concern we could change later.

dhruvkb

I can't approve this PR so please accept ✅ .

Co-authored-by: Zack Krida <[email protected]>

…pinia

obulat · 2022-03-02T14:53:13Z

I moved the nav and active-media stores to the /stores/ directory as is advised by the Pinia docs to differentiate them from Vuex modules.
I also renamed useNav to useNavStore, and added the navStore unit tests.
Now, I believe, this PR is ready to merge.

sarayourfriend

As mentioned by @zackkrida elsewhere there are errors in the app when trying to run it locally. None of the comments I added are blocking I don't think.

One thing I'd like to see is adding the two new stores to tsconfig.json (or updating them to be .ts). I tried this locally (converting to .ts) and there were only three errors to fix in active-media.ts which just came down to moving JSDoc parameter type annotations into TS annotations.

sarayourfriend · 2022-03-02T16:56:00Z

.eslintrc.js

@@ -1,3 +1,6 @@
+// WebStorm fix for `~` alias not working:
+// https://intellij-support.jetbrains.com/hc/en-us/community/posts/115000771544-ESLint-does-not-work-with-webpack-import-resolver-in-2017-3
+process.chdir(__dirname)


Wow! What a weird find.

Yes, definitely. It seems that the bug reports are from 2018, but I'm having this problem now 🤷

nuxt.config.ts

sarayourfriend · 2022-03-02T17:02:38Z

src/stores/active-media.js

+   * Only the properties used by components are exported as refs.
+   * `status` is not used anywhere in the components.
+   */
+  const { type, id, message } = toRefs(state)


I couldn't find any usages of activeMediaStore.type. id and message both have usages though.

I think type will be used when we have video, and we can have an activeVideo which will need to be treated differently from activeAudio

sarayourfriend · 2022-03-02T17:03:14Z

src/stores/active-media.js

+    type,
+    id,
+    message,


Should these all be readonly as well to prevent modifying them without going through the action functions?

src/stores/nav.js

sarayourfriend · 2022-03-02T17:10:54Z

src/stores/nav.js

+export const useNavStore = defineStore(NAV, () => {
+  const state = reactive({
+    isEmbedded: true,
+    isReferredFromCc: false,
+  })
+  const { isEmbedded, isReferredFromCc } = toRefs(state)
+
+  function setIsEmbedded(isEmbedded = true) {
+    state.isEmbedded = isEmbedded
+  }
+  function setIsReferredFromCc(isReferredFromCc = true) {
+    state.isReferredFromCc = isReferredFromCc
+  }
+  return {
+    isEmbedded,
+    isReferredFromCc,
+    setIsEmbedded,
+    setIsReferredFromCc,
+  }
+})


It looks like we use a mix of updating the isEmbedded and isReferredFromCc values directly and also through the setter functions. Should we standardize on one approach for simple properties like this?

openverse-frontend/src/middleware/middleware.js

Line 33 in 7c59473

navStore.setIsReferredFromCc(false)

openverse-frontend/src/plugins/migration-notice.js

Line 11 in 7c59473

navStore.isReferredFromCc = query.referrer.includes('creativecommons.org')

I prefer setter functions because they are easier to observe.

If we went with setter functions is there a way to enforce their usage, like to make isReferredFromCc readonly?

Yes! I'll do that

sarayourfriend · 2022-03-02T17:11:47Z

test/unit/specs/stores/active-media-store.spec.js

+
+describe('Active Media Store', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia())


Does this have the effect of resetting the store before each test?

Yes:
// creates a fresh pinia and make it active so it's automatically picked
// up by any useStore() call without having to pass it to it:
// useStore(pinia)

Co-authored-by: sarayourfriend <[email protected]>

…pinia

zackkrida · 2022-03-02T23:47:11Z

Reconfirming that this looks good! LGTM.

# Conflicts: # src/components/VFilters/VFilterChecklist.vue # src/locales/po-files/openverse.pot

dhruvkb added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 💻 aspect: code Concerns the software code in the repository labels Feb 18, 2022

dhruvkb changed the title ~~Pinia Poc~~ Create a proof-of-concept for Pinia migration Feb 18, 2022

sarayourfriend reviewed Feb 18, 2022

View reviewed changes

src/store/attribution.js Outdated Show resolved Hide resolved

krysal reviewed Feb 18, 2022

View reviewed changes

src/store/attribution.js Outdated Show resolved Hide resolved

dhruvkb added 2 commits February 21, 2022 11:24

Install Pinia

99142ea

Update the active media store to use Pinia

b30c432

dhruvkb force-pushed the pinia branch from 72549f0 to b30c432 Compare February 21, 2022 08:07

dhruvkb added 3 commits February 21, 2022 12:45

Delete the Vuex store

361730e

Update type definition and use it

9989251

Document the store composition variable

c2e2df5

dhruvkb force-pushed the pinia branch from 32fc2b7 to c2e2df5 Compare February 21, 2022 08:47

dhruvkb added 3 commits February 21, 2022 12:50

Fix a lint error that Prettier did not catch locally

61945b9

Use ~ instead of @ in imports

846b0c2

Update the nav store to use Pinia

c4221bb

dhruvkb force-pushed the pinia branch from 3ef3422 to c4221bb Compare February 21, 2022 10:36

Merge branch 'main' into pinia

299f255

sarayourfriend reviewed Feb 21, 2022

View reviewed changes

dhruvkb added 4 commits February 21, 2022 19:35

Rename store

5173d09

Make mutation payload fields non-nullable

46b1639

Merge branch 'main' into pinia

1ce982a

Fix leftover incorrect usage of the active media store

2c9c8ba

dhruvkb marked this pull request as ready for review February 22, 2022 04:24

dhruvkb requested a review from a team as a code owner February 22, 2022 04:24

Merge branch 'main' into pinia

8a218f0

# Conflicts: # src/locales/po-files/openverse.pot

sarayourfriend mentioned this pull request Mar 1, 2022

Create useUiStateCookie composable #974

Closed

1 task

obulat added 2 commits March 2, 2022 06:11

merge main

580b96f

fix possibly undefined

113e46c

zackkrida mentioned this pull request Mar 2, 2022

Convert the search store to Pinia #1024

Closed

7 tasks

zackkrida reviewed Mar 2, 2022

View reviewed changes

nuxt.config.ts Show resolved Hide resolved

zackkrida approved these changes Mar 2, 2022

View reviewed changes

zackkrida requested a review from obulat March 2, 2022 03:56

Add tests and other small fixes

0db4452

dhruvkb commented Mar 2, 2022

View reviewed changes

obulat and others added 3 commits March 2, 2022 17:27

Update nuxt.config.ts

e9f017e

Co-authored-by: Zack Krida <[email protected]>

Move converted modules to /stores/ and add unit tests

7c59473

Merge branch 'pinia' of github.com:WordPress/openverse-frontend into …

5934ff1

…pinia

Fix linting error

819ad80

obulat approved these changes Mar 2, 2022

View reviewed changes

sarayourfriend reviewed Mar 2, 2022

View reviewed changes

obulat and others added 7 commits March 2, 2022 20:38

Update nuxt.config.ts

7c8bd91

Co-authored-by: sarayourfriend <[email protected]>

Add changes from review

5820d15

Register component

dcfcfb2

Merge branch 'pinia' of github.com:WordPress/openverse-frontend into …

cf4229b

…pinia

Make properties readonly

6942747

Merge branch 'main' into pinia

66fd73b

Remove dublicates

d701ee9

Merge branch 'main' into pinia

4647efe

# Conflicts: # src/components/VFilters/VFilterChecklist.vue # src/locales/po-files/openverse.pot

obulat merged commit 912f1c4 into main Mar 3, 2022

obulat deleted the pinia branch March 3, 2022 03:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a proof-of-concept for Pinia migration #906

Create a proof-of-concept for Pinia migration #906

dhruvkb commented Feb 18, 2022 •

edited

Loading

sarayourfriend commented Feb 18, 2022

dhruvkb commented Feb 18, 2022 •

edited

Loading

zackkrida commented Feb 18, 2022

sarayourfriend commented Feb 21, 2022

sarayourfriend left a comment •

edited

Loading

zackkrida left a comment

dhruvkb left a comment

obulat commented Mar 2, 2022

sarayourfriend left a comment

sarayourfriend Mar 2, 2022

obulat Mar 2, 2022

sarayourfriend Mar 2, 2022

obulat Mar 2, 2022

sarayourfriend Mar 2, 2022

sarayourfriend Mar 2, 2022

obulat Mar 2, 2022

zackkrida Mar 2, 2022

obulat Mar 2, 2022

sarayourfriend Mar 2, 2022

obulat Mar 2, 2022

zackkrida commented Mar 2, 2022

Create a proof-of-concept for Pinia migration #906

Create a proof-of-concept for Pinia migration #906

Conversation

dhruvkb commented Feb 18, 2022 • edited Loading

Migration to Pinia

Goals

Side-goals

Steps

Conventions

Nomenclature

Type hinting

Procedure

Fixes

PR description

Checklist

Developer Certificate of Origin

sarayourfriend commented Feb 18, 2022

dhruvkb commented Feb 18, 2022 • edited Loading

zackkrida commented Feb 18, 2022

sarayourfriend commented Feb 21, 2022

sarayourfriend left a comment • edited Loading

Choose a reason for hiding this comment

zackkrida left a comment

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

obulat commented Mar 2, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackkrida commented Mar 2, 2022

dhruvkb commented Feb 18, 2022 •

edited

Loading

dhruvkb commented Feb 18, 2022 •

edited

Loading

sarayourfriend left a comment •

edited

Loading