Use Vuex map- helpers in components #242

obulat · 2021-09-18T07:26:40Z

Fixes

Related #205 by @dhruvkb and #229 by @obulat

Description

In an attempt to break the huge #229 into two parts, I extracted all the changes in the components into this PR.

Technical details

This PR contains many small refactorings replacing direct store calls (this.$store.dispatch('actionName')) with mapped versions (mapActions, mapMutations and mapState) in order to simplify component code.
Some actions here use a seemingly unnecessary string interpolation (this.commit(`${MUTATION_NAME_CONSTANT}`)). In fact, they are left this way to make adding module name easier in #229.

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 master).
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.

zackkrida

I've tested this a ton locally, everything is working great 😻

dhruvkb

This is a very massive undertaking. Thanks for simplifying this!

I've pointed out some changes, mostly including stylistic changes but one bug as well. These occur several times in the PR but I'm only highlighting one occurrence for brevity.

dhruvkb · 2021-09-20T03:50:58Z

src/components/AudioResultsList.vue

-      return this.$store.state.audios
-    },
+    ...mapState({
+      audios: (state) => state.audios,


Arrow functions that map to state properties can be replaced with strings:

Suggested change

audios: (state) => state.audios,

audios: 'audios',

The strings do not work for namespaced stores, so for consistency I used the same syntax everywhere.
I thought that only Webstorm for some reason doesn't like dots in mapState (and warns about them), but apparently it's a Vuex limitation, see vuejs/vuex#459 and vuejs/vuex#1592

For namespaced stores, the syntax is to use /, not . as the separator.

I tried out different versions of state mapping in both WebStorm and Visual Studio Code, here are some results I get:

...mapState(REPORT_CONTENT, { isReportFormVisible: 'isReportFormVisible' })
(+) Clear, easy to set for namespaced modules.
(-) Error-prone: No autocomplete in either WebStorm or Visual Studio Code, the property is not a constant
(-) Webstorm shows a warning about 'isReportFormVisible': "Cannot resolve Vuex state isReportFormVisible"
(-) Not clear if it works for nested objects

...mapState(REPORT_CONTENT, { isReportFormVisible: (state) => state.isReportFormVisible, }),
(-) Verbose
(+) Easier to avoid errors: both WebStorm and Visual Studio Code autocomplete the property after the dot (isReportFormVisible)
(+) No warnings.

...mapState(REPORT_CONTENT, ['isReportFormVisible'])
(+) Short
(-) Error prone: No autocomplete in either WebStorm or Visual Studio Code, the property is not a constant
(-) WebStorm warns that it "Cannot resolve Vuex state isReportFormVisible".
(-) Does not work well for nested objects (such as the query object in the search store)

src/components/AudioResultsList.vue

dhruvkb · 2021-09-20T04:08:25Z

src/components/AudioResultsList.vue

+    ...mapActions({
+      fetchMedia: `${FETCH_MEDIA}`,
+    }),


See https://github.com/WordPress/openverse-frontend/pull/229/files#r711866608.

src/components/Filters/FilterChecklist.vue

src/components/Filters/FilterDisplay.vue

dhruv · 2021-09-20T17:31:51Z

Please correct @dhruvkb's handle in the PR description.

Co-authored-by: Dhruv Bhanushali <dhruv_b@live.com>

# Conflicts: # src/components/NotificationBanner.vue # src/pages/search.vue

Make most of the Vuex store modules namespaced

# Conflicts: # src/store-modules/related-media-store.js # src/store-modules/search-store.js

dhruvkb

Really massive undertaking with a lot of code quality improvements!

There are still a large number of direct calls to $store.state, $store.dispatch etc. Are they intentionally not using map* helpers?

dhruvkb · 2021-09-22T16:18:08Z

src/components/AudioTrack/AudioController.vue

@@ -107,7 +107,7 @@ export default {
    )
    // Sync status from store to parent
    watch(
-      () => [store.state.activeMediaType, store.state.activeMediaId],
+      () => [store.state['active-media'].type, store.state['active-media'].id],


Does Nuxt automatically use the file name? Can we use the activeMedia name for the module to avoid []?

Unfortunately, yes, Nuxt uses the file name, and we cannot use the camelCased version. We could rename the store module to active

That'll be a good idea! I would've liked if Nuxt offered to option to name the state but we've got to work with what we have.

state.active will be better than state['active-media'].

obulat · 2021-09-23T10:09:44Z

Really massive undertaking with a lot of code quality improvements!

There are still a large number of direct calls to $store.state, $store.dispatch etc. Are they intentionally not using map* helpers?

Yes, they are not using map* helpers because I haven't figured out how to test them. In tests, Vuex cannot find namespaced modules when map* helpers are used with module name, eg. mapActions(RELATED, {setRelatedMedia: SET_RELATED_MEDIA})

Use Vuex map- helpers in components

767f8cf

obulat requested a review from a team as a code owner September 18, 2021 07:26

obulat requested review from krysal and dhruvkb September 18, 2021 07:26

obulat added ✨ goal: improvement Improvement to an existing user-facing feature 💻 aspect: code Concerns the software code in the repository labels Sep 18, 2021

obulat added 9 commits September 18, 2021 10:29

Move action and mutation types to constants directory

fbb5166

Move related-media-store to 'store'

226ec67

Use namespaced 'related' store module

8631b4a

Make 'abtest' module namespaced

c0ff2fb

Make 'active-media' and 'attribution' modules namespaced

89100e3

Make 'bug-report' and 'provider' modules namespaced

5b17365

Make 'nav' module namespaced

9aa833d

Make 'notification' and 'report-content' modules namespaced

5b90ff7

Make 'social', 'user', 'usage-data' modules namespaced

f684eef

obulat mentioned this pull request Sep 19, 2021

Make most of the Vuex store modules namespaced #244

Merged

7 tasks

zackkrida approved these changes Sep 19, 2021

View reviewed changes

dhruvkb suggested changes Sep 20, 2021

View reviewed changes

obulat and others added 5 commits September 20, 2021 21:38

Update src/components/Filters/FilterDisplay.vue

b65b853

Co-authored-by: Dhruv Bhanushali <dhruv_b@live.com>

Fix bug with redefined audiosCount

2fcd89c

Apply changes from code review and cleanup

062d391

Merge branch 'use_map_helpers_in_components' into namespaced_store

0145ced

# Conflicts: # src/components/NotificationBanner.vue # src/pages/search.vue

Merge pull request #244 from WordPress/namespaced_store

8e9b7cd

Make most of the Vuex store modules namespaced

obulat mentioned this pull request Sep 21, 2021

Use namespaced modules for Vuex store #229

Closed

7 tasks

obulat added 2 commits September 22, 2021 06:44

Merge branch 'main' into use_map_helpers_in_components

2c2f615

# Conflicts: # src/store-modules/related-media-store.js # src/store-modules/search-store.js

Replace verbose mapState declarations

122443e

dhruvkb approved these changes Sep 22, 2021

View reviewed changes

Fix some store paths

9502b45

obulat merged commit 0449037 into main Sep 23, 2021

obulat deleted the use_map_helpers_in_components branch September 23, 2021 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Vuex map- helpers in components #242

Use Vuex map- helpers in components #242

obulat commented Sep 18, 2021 •

edited by dhruvkb

Loading

zackkrida left a comment

dhruvkb left a comment

dhruvkb Sep 20, 2021

obulat Sep 20, 2021

dhruvkb Sep 21, 2021

obulat Sep 21, 2021

dhruvkb Sep 20, 2021

dhruv commented Sep 20, 2021

dhruvkb left a comment

dhruvkb Sep 22, 2021 •

edited

Loading

obulat Sep 23, 2021

dhruvkb Sep 23, 2021

obulat commented Sep 23, 2021

Use Vuex map- helpers in components #242

Use Vuex map- helpers in components #242

Conversation

obulat commented Sep 18, 2021 • edited by dhruvkb Loading

Fixes

Description

Technical details

Checklist

Developer Certificate of Origin

zackkrida left a comment

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb Sep 20, 2021

Choose a reason for hiding this comment

obulat Sep 20, 2021

Choose a reason for hiding this comment

dhruvkb Sep 21, 2021

Choose a reason for hiding this comment

obulat Sep 21, 2021

Choose a reason for hiding this comment

dhruvkb Sep 20, 2021

Choose a reason for hiding this comment

dhruv commented Sep 20, 2021

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

obulat Sep 23, 2021

Choose a reason for hiding this comment

dhruvkb Sep 23, 2021

Choose a reason for hiding this comment

obulat commented Sep 23, 2021

obulat commented Sep 18, 2021 •

edited by dhruvkb

Loading

dhruvkb Sep 22, 2021 •

edited

Loading