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 the main mail store to Pinia #10138

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

GVodyanov
Copy link
Contributor

@GVodyanov GVodyanov commented Sep 12, 2024

Fix #9237

This is going to be a big one...

Splitting stores into multiple files isn't very common in Pinia, but I really wanted to do so in this case, I took my inspiration for how to do it from these guys vuejs/pinia#802

A little overview of how this was done:

  • Created a subfolder in /store for all the main mail store stuff
  • Migrate all the getters and remove now useless ones (the ones that just point to a piece of state without modifying it)
  • Migrate the actions file
  • Rename all the mutations to [previous name] + Mutation
  • Migrate dispatch and commit usages in components to the correct Pinia usage
  • Move the renamed mutations into the actions file, hence making them actions (and this time I didn't inline anything, at least yet)
  • Realized that a lot of the getters had parameters, which you can't have in Pinia, so those become actions as well (not renamed)
  • Lots of bugfixing
  • Migrate the tests (the component ones are easy, just updating references, however the ones that actually test the store itself require pretty significant modification)

@GVodyanov GVodyanov added enhancement 2. developing skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills labels Sep 12, 2024
@GVodyanov GVodyanov self-assigned this Sep 12, 2024
@GVodyanov GVodyanov force-pushed the feat/migrate-main-store-pinia branch from 18396da to b8e63c1 Compare September 17, 2024 08:01
@GVodyanov GVodyanov force-pushed the feat/migrate-main-store-pinia branch from 76a519f to 78247eb Compare September 17, 2024 13:51
@GVodyanov GVodyanov force-pushed the feat/migrate-main-store-pinia branch from 78247eb to 117f534 Compare September 17, 2024 14:51
@GVodyanov
Copy link
Contributor Author

Not opening the PR yet as I still have the tests to migrate, but mail is useable (as far as it seems)

@GVodyanov
Copy link
Contributor Author

Thanks so much for finishing the hardest parts Richard <3

@GVodyanov GVodyanov marked this pull request as ready for review October 10, 2024 14:13
@GVodyanov GVodyanov requested review from hamza221 and ChristophWurst and removed request for ChristophWurst, kesselb and GretaD October 10, 2024 14:13
@hamza221
Copy link
Contributor

needs a rebase, I wish you luck 🍀

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Should be merged after branch off for stable4.1

@kesselb kesselb dismissed their stale review November 6, 2024 14:35

branch off for stable4.1 done

@ChristophWurst
Copy link
Member

Let's get this in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace global Vuex store with Pinia
5 participants