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

Merged
merged 1 commit into from
Jan 13, 2025
Merged

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 3 times, most recently 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

@st3iny st3iny force-pushed the feat/migrate-main-store-pinia branch 3 times, most recently from 4325bfd to 1a5d97f Compare January 7, 2025 12:09
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Rebased, fixed conflicts and did some testing. I couldn't spot breakage.

@st3iny st3iny force-pushed the feat/migrate-main-store-pinia branch from 1a5d97f to 37381d8 Compare January 7, 2025 12:09
@st3iny

This comment was marked as resolved.

@st3iny st3iny added blocked and removed blocked labels Jan 7, 2025
@st3iny st3iny self-assigned this Jan 11, 2025
Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>

# Conflicts:
#	src/components/ThreadEnvelope.vue
#	src/main.js
#	src/store/getters.js
#	src/store/index.js
#	src/store/mutations.js
@GVodyanov GVodyanov force-pushed the feat/migrate-main-store-pinia branch from 1157541 to 34d7ce8 Compare January 11, 2025 18:40
@GVodyanov GVodyanov requested a review from st3iny January 11, 2025 18:40
@GVodyanov
Copy link
Contributor Author

Oh shoot I'm sorry I didn't see that you wanted to finish this too @st3iny. I hope I didn't step on your feet here, everything should work now though, if you could just give a final review.

@st3iny
Copy link
Member

st3iny commented Jan 11, 2025

No no, all good. I didn't do anything yet, just assigned myself to not forget about the PR.

Thanks for the final rebase :)

@st3iny st3iny merged commit eb92516 into main Jan 13, 2025
34 checks passed
@st3iny st3iny deleted the feat/migrate-main-store-pinia branch January 13, 2025 09:10
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