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.

Sign up for GitHub

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

Jump to bottom

Migrate/vuex to pinia #5705

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Migrate/vuex to pinia #5705

merged 1 commit into from
Jun 25, 2024

Conversation

GVodyanov
Copy link
Contributor

@GVodyanov GVodyanov commented Jan 21, 2024 •
edited
Loading

Close #5686 as part of nextcloud/groupware#64

Continuation of #5685

TODO:

  • Migrate setting store
  • Update settings store usage
  • Migrate principals store
  • Update principals store usage
  • Migrate importState store
  • Update importState store usage
  • Migrate importFiles store
  • Update importFiles store usage
  • Migrate fetchedTimeRanges store
  • Update fetchedTimeRanges store usage
  • Migrate davRestrictions store
  • Update davRestrictions store usage (not found..?)
  • Migrate contacts store
  • Update contacts store usage (not found..?)
  • Migrate calendars store
  • Update calendars store usage
  • Migrate calendarObjects store
  • Update calendarObjects store usage
  • Migrate calendarObjectsInstance store
  • Update calendarObjectsInstance store usage
  • Remove mapGetters, mapState, etc.
  • Remove direct $store references
  • Recreate unit tests for all the stores
  • Manual testing (in progress)

@GVodyanov GVodyanov self-assigned this Jan 21, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 21.04145% with 743 lines in your changes missing coverage. Please review.

Project coverage is 24.02%. Comparing base (c01de6e) to head (88f6dd2).
Report is 3 commits behind head on main.

Files Patch % Lines
src/store/calendars.js 6.13% 275 Missing and 31 partials ⚠️
src/store/calendarObjects.js 1.69% 102 Missing and 14 partials ⚠️
src/store/fetchedTimeRanges.js 10.71% 41 Missing and 9 partials ⚠️
src/store/principals.js 0.00% 27 Missing and 4 partials ⚠️
src/mixins/EditorMixin.js 0.00% 29 Missing and 1 partial ⚠️
src/store/settings.js 80.00% 18 Missing and 4 partials ⚠️
src/components/AppNavigation/Settings.vue 0.00% 17 Missing ⚠️
src/store/widget.js 15.38% 11 Missing ⚠️
src/components/Editor/Invitees/InviteesList.vue 0.00% 9 Missing ⚠️
src/views/Calendar.vue 0.00% 9 Missing ⚠️
... and 42 more
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #5705      +/-   ##
============================================
- Coverage     29.29%   24.02%   -5.28%     
+ Complexity      914      457     -457     
============================================
  Files           287      246      -41     
  Lines         13987    11569    -2418     
  Branches       2139     2163      +24     
============================================
- Hits           4098     2779    -1319     
+ Misses         9575     8475    -1100     
- Partials        314      315       +1
Flag Coverage Δ
javascript 15.56% <21.04%> (+0.37%) ⬆️
php 58.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@st3iny st3iny self-requested a review May 22, 2024 05:46
st3iny
st3iny requested changes May 22, 2024
View reviewed changes
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.

First batch

I tested most changes locally.

npm Outdated Show resolved Hide resolved
src/components/AppNavigation/CalendarList.vue Outdated Show resolved Hide resolved
src/store/calendars.js Outdated Show resolved Hide resolved
src/components/AppNavigation/CalendarList/CalendarListItem.vue Outdated Show resolved Hide resolved
src/components/AppNavigation/CalendarList/CalendarListItem.vue Outdated Show resolved Hide resolved
src/store/calendars.js Outdated Show resolved Hide resolved
src/store/calendars.js Outdated Show resolved Hide resolved
src/store/importFiles.js Outdated Show resolved Hide resolved
src/components/CalendarGrid.vue Outdated Show resolved Hide resolved
src/components/CalendarGrid.vue Outdated Show resolved Hide resolved
@GVodyanov GVodyanov marked this pull request as ready for review May 23, 2024 17:39
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 24, 2024
st3iny
st3iny previously requested changes May 24, 2024
View reviewed changes
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.

Second batch. One more left.

src/mixins/EditorMixin.js Outdated Show resolved Hide resolved
src/mixins/EditorMixin.js Outdated Show resolved Hide resolved
st3iny
st3iny reviewed May 24, 2024
View reviewed changes
Copy link
Member

@st3iny st3iny left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Third batch done. Now, only manual testing and rebasing is left to be done.

@st3iny st3iny force-pushed the Migrate/vuex-to-pinia branch from fc48105 to 3ae431e Compare May 31, 2024 13:33
@st3iny
Copy link
Member

st3iny commented May 31, 2024

I rebased and tried to migrate the new code which would be Hamza's widgets and my default calendar feature. I clicked around a bit and already fixed some console errors.

The only thing left to do is manual testing. I'd prefer to get it in quickly to prevent ending up in conflict hell.

@st3iny st3iny dismissed their stale review May 31, 2024 13:38

I reviewed and rebased the PR.

@GVodyanov
Copy link
Contributor Author

I also did some clicking around and everything seems to be working well, thanks @st3iny!

@GVodyanov GVodyanov requested review from GretaD and hamza221 June 13, 2024 10:15
@st3iny st3iny force-pushed the Migrate/vuex-to-pinia branch from 3ae431e to 7de1908 Compare June 24, 2024 09:40
@st3iny st3iny requested a review from ChristophWurst June 24, 2024 09:52
@miaulalala
Copy link
Contributor

Error in ListView when opening the sidebar for a readonly event:

[Vue warn]: Missing required prop: "name"

found in

---> <NcAppSidebar>
       <EditSidebar> at src/views/EditSidebar.vue
         <NcContent>
           <Calendar> at src/views/Calendar.vue
             <App> at src/App.vue
               <Root> vue.runtime.esm.js:4625
[Vue warn]: Error in mounted hook: "TypeError: this.$refs.header is undefined"

found in

---> <NcAppSidebar>
       <EditSidebar> at src/views/EditSidebar.vue
         <NcContent>
           <Calendar> at src/views/Calendar.vue
             <App> at src/App.vue
               <Root> vue.runtime.esm.js:4625
TypeError: this.$refs.header is undefined
    focus NcAppSidebar-CQWODfsy.mjs:681
    mounted NcAppSidebar-CQWODfsy.mjs:562
    VueJS 18
    init vue-router.esm.js:3005
    init vue-router.esm.js:3004
    updateRoute vue-router.esm.js:2414
    transitionTo vue-router.esm.js:2263
    confirmTransition vue-router.esm.js:2402
    step vue-router.esm.js:2084
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    routeEnterGuard vue-router.esm.js:2542
    beforeRouteEnter EditorMixin.js:686
    routeEnterGuard vue-router.esm.js:2535
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    runQueue vue-router.esm.js:2095
    confirmTransition vue-router.esm.js:2397
    step vue-router.esm.js:2084
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    resolveAsyncComponents vue-router.esm.js:2162
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    step vue-router.esm.js:2091
    step vue-router.esm.js:2091
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    created CalendarGrid.vue:235
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    __WEBPACK_DEFAULT_EXPORT__ windowTitleService.js:42
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    runQueue vue-router.esm.js:2095
    confirmTransition vue-router.esm.js:2392
    transitionTo vue-router.esm.js:2260
    push vue-router.esm.js:2606
    push vue-router.esm.js:3036
    push vue-router.esm.js:3035
    handleEventClick eventClick.js:82
    __WEBPACK_DEFAULT_EXPORT__ eventClick.js:37
    trigger internal-common.js:3680
    node_modules calendar-main.js:183014
    node_modules calendar-main.js:183880
    listenBySelector internal-common.js:270
    EventClicking index.js:1783
    node_modules calendar-main.js:183098
    node_modules calendar-main.js:183098
    node_modules calendar-main.js:194522
    setRef internal-common.js:2380
    node_modules calendar-main.js:186150
    Preact 22
vue.runtime.esm.js:3065
[Vue warn]: Missing required prop: "name"

found in

---> <NcAppSidebar>
       <EditSidebar> at src/views/EditSidebar.vue
         <NcContent>
           <Calendar> at src/views/Calendar.vue
             <App> at src/App.vue
               <Root> vue.runtime.esm.js:4625
[Vue warn]: onMounted is called when there is no active component instance to be associated with. Lifecycle injection APIs can only be used during execution of setup().

found in

---> <NcDialog>
       <AttachmentsList> at src/components/Editor/Attachments/AttachmentsList.vue
         <NcAppSidebarTab>
           <NcAppSidebarTabs>
             <Fragment>
               <NcAppSidebar>
                 <EditSidebar> at src/views/EditSidebar.vue
                   <NcContent>
                     <Calendar> at src/views/Calendar.vue
                       <App> at src/App.vue
                         <Root> vue.runtime.esm.js:4625
[Vue warn]: Missing required prop: "showHeader"

found in

---> <InviteesList> at src/components/Editor/Invitees/InviteesList.vue
       <NcAppSidebarTab>
         <NcAppSidebarTabs>
           <Fragment>
             <NcAppSidebar>
               <EditSidebar> at src/views/EditSidebar.vue
                 <NcContent>
                   <Calendar> at src/views/Calendar.vue
                     <App> at src/App.vue
                       <Root> vue.runtime.esm.js:4625

Same for month view:

[Vue warn]: Missing required prop: "name"

found in

---> <NcAppSidebar>
       <EditSidebar> at src/views/EditSidebar.vue
         <NcContent>
           <Calendar> at src/views/Calendar.vue
             <App> at src/App.vue
               <Root> vue.runtime.esm.js:4625
[Vue warn]: Error in mounted hook: "TypeError: this.$refs.header is undefined"

found in

---> <NcAppSidebar>
       <EditSidebar> at src/views/EditSidebar.vue
         <NcContent>
           <Calendar> at src/views/Calendar.vue
             <App> at src/App.vue
               <Root> vue.runtime.esm.js:4625
TypeError: this.$refs.header is undefined
    focus NcAppSidebar-CQWODfsy.mjs:681
    mounted NcAppSidebar-CQWODfsy.mjs:562
    VueJS 18
    init vue-router.esm.js:3005
    init vue-router.esm.js:3004
    updateRoute vue-router.esm.js:2414
    transitionTo vue-router.esm.js:2263
    confirmTransition vue-router.esm.js:2402
    step vue-router.esm.js:2084
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    routeEnterGuard vue-router.esm.js:2542
    beforeRouteEnter EditorMixin.js:686
    routeEnterGuard vue-router.esm.js:2535
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    runQueue vue-router.esm.js:2095
    confirmTransition vue-router.esm.js:2397
    step vue-router.esm.js:2084
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    resolveAsyncComponents vue-router.esm.js:2162
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    step vue-router.esm.js:2091
    step vue-router.esm.js:2091
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    created CalendarGrid.vue:235
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    __WEBPACK_DEFAULT_EXPORT__ windowTitleService.js:42
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    runQueue vue-router.esm.js:2095
    confirmTransition vue-router.esm.js:2392
    transitionTo vue-router.esm.js:2260
    push vue-router.esm.js:2606
    push vue-router.esm.js:3036
    push vue-router.esm.js:3035
    handleEventClick eventClick.js:82
    __WEBPACK_DEFAULT_EXPORT__ eventClick.js:37
    trigger internal-common.js:3680
    node_modules calendar-main.js:183014
    node_modules calendar-main.js:183880
    listenBySelector internal-common.js:270
    EventClicking index.js:1783
    node_modules calendar-main.js:183098
    node_modules calendar-main.js:183098
    registerInteractiveComponent internal.js:761
    componentDidMount internal.js:747
    Preact 4
    node_modules calendar-main.js:183260
    flushSync internal-common.js:2229
    node_modules calendar-main.js:183259
    drained internal-common.js:156
    tryDrain internal-common.js:138
    resume internal-common.js:127
    batchRendering index.js:2070
    changeView internal-common.js:4805
    created CalendarGrid.vue:228
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    step vue-router.esm.js:2088
    iterator vue-router.esm.js:2384
    __WEBPACK_DEFAULT_EXPORT__ windowTitleService.js:42
    iterator vue-router.esm.js:2362
    step vue-router.esm.js:2087
    runQueue vue-router.esm.js:2095
vue.runtime.esm.js:3065
[Vue warn]: Missing required prop: "name"

found in

---> <NcAppSidebar>
       <EditSidebar> at src/views/EditSidebar.vue
         <NcContent>
           <Calendar> at src/views/Calendar.vue
             <App> at src/App.vue
               <Root> vue.runtime.esm.js:4625
[Vue warn]: onMounted is called when there is no active component instance to be associated with. Lifecycle injection APIs can only be used during execution of setup().

found in

---> <NcDialog>
       <AttachmentsList> at src/components/Editor/Attachments/AttachmentsList.vue
         <NcAppSidebarTab>
           <NcAppSidebarTabs>
             <Fragment>
               <NcAppSidebar>
                 <EditSidebar> at src/views/EditSidebar.vue
                   <NcContent>
                     <Calendar> at src/views/Calendar.vue
                       <App> at src/App.vue
                         <Root> vue.runtime.esm.js:4625
[Vue warn]: Missing required prop: "showHeader"

found in

---> <InviteesList> at src/components/Editor/Invitees/InviteesList.vue
       <NcAppSidebarTab>
         <NcAppSidebarTabs>
           <Fragment>
             <NcAppSidebar>
               <EditSidebar> at src/views/EditSidebar.vue
                 <NcContent>
                   <Calendar> at src/views/Calendar.vue
                     <App> at src/App.vue
                       <Root> vue.runtime.esm.js:4625

@st3iny
Copy link
Member

st3iny commented Jun 25, 2024

Nice finds. Those are regressions from the nc-vue v8 migration and calendar editor redesign. I'll create a new PR.

I also just pushed a commit to fix uploading attachments.

@st3iny st3iny mentioned this pull request Jun 25, 2024
Closed
@GVodyanov @st3iny
chore: migrate from Vuex to Pinia
88f6dd2 
Signed-off-by: Grigory V <[email protected]>
Signed-off-by: Richard Steinmetz <[email protected]>
@st3iny st3iny force-pushed the Migrate/vuex-to-pinia branch from a87546a to 88f6dd2 Compare June 25, 2024 14:58
st3iny
st3iny approved these changes Jun 25, 2024
View reviewed changes
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.

Let's go!!! 🚀 🌔

@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 25, 2024
@st3iny st3iny enabled auto-merge June 25, 2024 17:20
@st3iny st3iny merged commit 8b5c9f4 into main Jun 25, 2024
40 of 42 checks passed
@st3iny st3iny deleted the Migrate/vuex-to-pinia branch June 25, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st3iny st3iny st3iny approved these changes

@GretaD GretaD Awaiting requested review from GretaD

@hamza221 hamza221 Awaiting requested review from hamza221

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@GVodyanov GVodyanov

Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace global Vuex store with Pinia
4 participants
@GVodyanov @st3iny @miaulalala @ChristophWurst