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

feat(store) replace Vuex settingsStore with equivalent Pinia store #9892

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jul 3, 2023

☑️ Resolves

  • Part of Vue 3 Migration #9448
  • This PR as a beginner should be a proper example of how to migrate other stores - feel free to suggest changes!

🖼️ Screenshots

  • No visual changes
  • Only NewMessage Typing indicator and Talk settings private statuses switches are affected

🚧 Tasks

  • Manual testing
  • Code review

🏁 Checklist

@Antreesy Antreesy added 3. to review enhancement feature: frontend 🖌️ "Web UI" client dependencies Pull requests that update a dependency file labels Jul 3, 2023
@Antreesy Antreesy added this to the 💜 Next Major (28) milestone Jul 3, 2023
@Antreesy Antreesy requested review from ShGKme and DorraJaouad July 3, 2023 18:14
@Antreesy Antreesy self-assigned this Jul 3, 2023
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

It seems that we also need to init Pinia at least in files sidebar app. And then we will need to init pinia in other apps used store.

All the other things looks great to me.


While I believe, moving to Pinia has only benefits and no drawbacks, I'm not sure we should do it now (as was discussed with @nickvergessen on the Talk Team week).

This is not a part of Vue 3 migration as both Pinia and Vuex are not linked to a specific Vue version. We can do it before the migration as well as after the migration.

Nevertheless, I'm personally fine to move to Pinia any time.

src/main.js Show resolved Hide resolved
src/stores/settingsStore.js Outdated Show resolved Hide resolved
@Antreesy Antreesy force-pushed the feat/9448/add-pinia-stores branch from 19d3eca to d9c42f6 Compare July 13, 2023 18:11
@Antreesy Antreesy requested a review from ShGKme July 13, 2023 18:12
src/stores/settings.js Outdated Show resolved Hide resolved
@Antreesy Antreesy force-pushed the feat/9448/add-pinia-stores branch from d9c42f6 to 578e5bf Compare July 17, 2023 13:04
Antreesy added 2 commits July 17, 2023 17:12
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the feat/9448/add-pinia-stores branch from 578e5bf to ec5dd42 Compare July 17, 2023 15:53
@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 17, 2023

Rebased onto master because of conflicting package-lock.json

Also reverted prevously discussed changes regarding storing the capabilities

@Antreesy Antreesy requested a review from ShGKme July 17, 2023 15:55
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

🍍 Tested, works fine 🍍

Note, OptionsApi-like define store was used here. It is easier for migration purpose and looks more structured. But CompositionApi-like defineStore have more possibilities, such as:

  • More complex init
  • Internal (aka private) methods and variables
  • Possibility to use watch and etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review dependencies Pull requests that update a dependency file enhancement feature: frontend 🖌️ "Web UI" client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants