Convert `provider` store from Vuex to Pinia #1158

obulat · 2022-03-22T11:25:24Z

Fixes

Fixes #1027 by @obulat
Fixes #967 by @sarayourfriend

This PR was edited to convert the setup store to an options store to fix the server state hydration issues. Please, re-review :)

Description

This PR converts the provider store to Pinia. Provider stats include provider short name and display name, URL, as well as the media count per each provider. These data are used in the provider filters for image/audio, and on the sources page.

The state is simplified similar to the media store state to enable simple access to properties for various media types without string manipulation (e.g., fetchState[mediaType].isFetching instead of isFetching${titleCase(mediaType)}Providers).

Previously, we fetched provider data once, during the server initialization, using nuxtServerInit hook. Pinia does not have a direct analog. Here, I used a middleware to call fetchMediaProviders before every route (found in a Pinia discussion).
To minimize the number of unnecessary requests, I set a lastUpdated timestamp in the store as a ssrRef. When calling fetchMediaProviders, we only actually fetch data from the API if the provider array is empty, or more than 1 hour has passed since lastUpdated.
This way provider store loads all the provider stats for each request, and updates them if necessary once an hour.

Testing Instructions

Run the app, check the provider filters in the sidebar, they should work for images and audio.
Sources pages should display the image providers (note that audio providers were not yet added to this page).
Try disabling the network, you should see a warning in the console that providers were not loaded.

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 a parent feature branch.
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.

src/middleware/middleware.js

src/stores/provider.js

krysal

This looks great! I was worried this module would make too many requests for providers that actually rarely change, thanks for adding the check if it needs an update or not ⭐️

src/store/index.js

test/unit/specs/stores/provider.spec.js

src/stores/provider.ts

sarayourfriend · 2022-04-04T07:25:33Z

src/middleware/middleware.ts

+export default async function ({
+  query,
+  route,
+  $pinia,
+}: {
+  query: Record<string, string>
+  route: Route
+  $pinia: Pinia
+}): Promise<void> {


I guess this conflicts with #1228, should we merge that first then update this afterwards to use the standard implementation for the types?

sarayourfriend · 2022-04-04T07:28:19Z

src/stores/provider.ts

+/**
+ * Timestamp is used to limit the update frequency to one every 60 minutes per request.
+ */
+const lastUpdated: Ref<Date | null> = ssrRef(null)
+
+const updateFrequency = process.env.providerUpdateFrequency
+  ? parseInt(process.env.providerUpdateFrequency, 10)
+  : 60 * 60 * 1000


Was this necessary in the end? I thought it was agreed to just make the request for the providers list on every request to SSR?

Yes, we've decided to make the request for the providers list on every request to SSR. However, without this check, it was making more than one provider request per SSR. It was updating on the server and on the client, too.

it was making more than one provider request per SSR

Is this because multiple places request it? Is there somehow we could centralize it per request 🤔 Like adding it to the request context or something I guess?

sarayourfriend

LGTM! Some minor typescript stuff but tested locally and it works great.

src/stores/provider.ts

test/unit/specs/stores/provider.spec.js

src/stores/provider.ts

sarayourfriend · 2022-04-11T08:28:26Z

src/stores/provider.ts

+    }
+
+    const timeSinceLastUpdate =
+      new Date().getTime() - new Date(lastUpdated.value).getTime()


TIL getTime and valueOf on Date are equivalent!

Interesting!

…he search store easier

sarayourfriend

Looks great! Tested well locally 😁

sarayourfriend · 2022-04-13T16:37:14Z

src/models/media.ts

@@ -28,6 +28,8 @@ export interface Media {

  provider: string
  source?: string
+  providerName?: string


How does providerName get populated? And should it be snake case if it's coming from the API?

I see the API mapping provider_name field to display_name in the provider serializer: https://github.com/WordPress/openverse-api/blob/a7955c86d43bff504e8d41454f68717d79dd3a44/api/catalog/api/serializers/provider_serializers.py#L11-L14

But not on the media serializer 🤔

Good question, I wonder the same ❓

providerName is generated when fetching the media item (only when fetching the single item, because I guess search views do not require it?):

openverse-frontend/src/stores/media/index.ts

Lines 343 to 351 in 73d0dac

mediaDetail.providerName = providerStore.getProviderName(

mediaDetail.provider,

mediaType

)

if (mediaDetail.source) {

mediaDetail.sourceName = providerStore.getProviderName(

mediaDetail.source,

mediaType

)

I guess we could also split the interface into API Media and Local Media if that makes it clearer in the future.

sarayourfriend · 2022-04-13T16:41:25Z

src/utils/pinia-state.ts

@@ -0,0 +1,60 @@
+import { isClient } from '~/composables/window'


We can remove this module now right?

sarayourfriend · 2022-04-13T16:43:07Z

test/unit/specs/stores/provider.spec.js

+  afterEach(() => {
+    afterEach(() => {
+      providerServices.audio.getProviderStats.mockClear()
+      providerServices.image.getProviderStats.mockClear()
+    })
+  })


Suggested change

afterEach(() => {

afterEach(() => {

providerServices.audio.getProviderStats.mockClear()

providerServices.image.getProviderStats.mockClear()

})

})

afterEach(() => {

providerServices.audio.getProviderStats.mockClear()

providerServices.image.getProviderStats.mockClear()

})

sarayourfriend · 2022-04-13T16:44:16Z

src/stores/provider.ts

+  }
+}
+
+export const initialProviderState: ProviderState = Object.freeze({


Should we use deepFreeze here since it's a nested structure?

sarayourfriend · 2022-04-13T16:44:58Z

src/stores/provider.ts

+  state: (): ProviderState => ({
+    providers: {
+      [AUDIO]: [],
+      [IMAGE]: [],
+    },
+    fetchState: {
+      [AUDIO]: { ...initialFetchState },
+      [IMAGE]: { ...initialFetchState },
+    },
+  }),


This can use the initialProviderState object, right? Just need to deepclone it if it's frozen.

I removed the initialProviderState object altogether, because it was overcomplicating things (deepFreeze/deepclone), and not really serving much purpose.

krysal

I tested it with changes of main and everything seems to work well :) Nice work!

krysal · 2022-04-13T19:43:05Z

src/models/media.ts

@@ -28,6 +28,8 @@ export interface Media {

  provider: string
  source?: string
+  providerName?: string


Good question, I wonder the same ❓

obulat added 🟨 priority: medium Not blocking but should be addressed soon 💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Mar 22, 2022

obulat added this to the Switch from Vuex to Pinia milestone Mar 22, 2022

obulat requested a review from a team as a code owner March 22, 2022 11:25

obulat requested review from krysal and sarayourfriend March 22, 2022 11:25

sarayourfriend reviewed Mar 22, 2022

View reviewed changes

src/middleware/middleware.js Outdated Show resolved Hide resolved

src/stores/provider.js Outdated Show resolved Hide resolved

src/stores/provider.js Outdated Show resolved Hide resolved

Base automatically changed from pinia_search_convert_search_store to main March 22, 2022 13:51

krysal approved these changes Mar 31, 2022

View reviewed changes

src/store/index.js Show resolved Hide resolved

test/unit/specs/stores/provider.spec.js Outdated Show resolved Hide resolved

test/unit/specs/stores/provider.spec.js Outdated Show resolved Hide resolved

src/stores/provider.ts Outdated Show resolved Hide resolved

obulat force-pushed the pinia_provider branch from 67ff292 to d89cc40 Compare April 2, 2022 04:24

sarayourfriend reviewed Apr 4, 2022

View reviewed changes

obulat mentioned this pull request Apr 4, 2022

TS-ify plugins and middleware #1228

Merged

7 tasks

sarayourfriend approved these changes Apr 11, 2022

View reviewed changes

obulat mentioned this pull request Apr 11, 2022

Correctly hydrating the server state in Pinia setup stores #1254

Closed

1 task

obulat force-pushed the pinia_provider branch from 11e314c to a4a3e74 Compare April 12, 2022 07:50

Convert provider store to Pinia

517211d

obulat force-pushed the pinia_provider branch from a4a3e74 to 517211d Compare April 12, 2022 07:55

obulat added 2 commits April 12, 2022 11:01

Revert types file move to make PR smaller

3b9abc5

Convert Provider to an options store

b4ceef9

obulat force-pushed the pinia_provider branch from 59353ad to b4ceef9 Compare April 12, 2022 14:31

Try increasing the Storybook Playwright timeout

27f58d2

obulat requested review from krysal and sarayourfriend April 12, 2022 14:39

Decrease Storybook Playwright WebServer timeout; add tapes

b87bde2

Init provider filters from the provider store to make conversion of t…

9166a0c

…he search store easier

sarayourfriend approved these changes Apr 13, 2022

View reviewed changes

krysal approved these changes Apr 13, 2022

View reviewed changes

obulat added 2 commits April 14, 2022 06:21

Merge branch 'main' into pinia_provider

73d0dac

Remove unnecessary initialProviderState

f4b57dd

obulat merged commit 9e4893e into main Apr 14, 2022

obulat deleted the pinia_provider branch April 14, 2022 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `provider` store from Vuex to Pinia #1158

Convert `provider` store from Vuex to Pinia #1158

obulat commented Mar 22, 2022 •

edited

Loading

krysal left a comment

sarayourfriend Apr 4, 2022

sarayourfriend Apr 4, 2022

obulat Apr 4, 2022

sarayourfriend Apr 4, 2022

sarayourfriend left a comment

sarayourfriend Apr 11, 2022

obulat Apr 11, 2022

sarayourfriend left a comment

sarayourfriend Apr 13, 2022

krysal Apr 13, 2022

obulat Apr 14, 2022

sarayourfriend Apr 13, 2022

sarayourfriend Apr 13, 2022

sarayourfriend Apr 13, 2022

sarayourfriend Apr 13, 2022

obulat Apr 14, 2022

krysal left a comment

krysal Apr 13, 2022

	mediaDetail.providerName = providerStore.getProviderName(
	mediaDetail.provider,
	mediaType
	)
	if (mediaDetail.source) {
	mediaDetail.sourceName = providerStore.getProviderName(
	mediaDetail.source,
	mediaType
	)

		@@ -0,0 +1,60 @@
		import { isClient } from '~/composables/window'

Convert provider store from Vuex to Pinia #1158

Convert provider store from Vuex to Pinia #1158

Conversation

obulat commented Mar 22, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

krysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Convert `provider` store from Vuex to Pinia #1158

Convert `provider` store from Vuex to Pinia #1158

obulat commented Mar 22, 2022 •

edited

Loading