-
Notifications
You must be signed in to change notification settings - Fork 391
upcoming: [M3-9225] - Add API queries for MarketplaceV2 #13255
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
base: develop
Are you sure you want to change the base?
upcoming: [M3-9225] - Add API queries for MarketplaceV2 #13255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to this PR, but it might be worth fixing here: I noticed the factory mocks & Types don't fully match the API doc
logo_url_night_modeand its value should be updated to matchlogo_url_dark_modeproduct_countbe changed toproducts_countat L27 & L34
| import type { Filter, Params } from '@linode/api-v4'; | ||
|
|
||
| export const marketplaceQueries = createQueryKeys('marketplace', { | ||
| product: (productId: number) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now the api has changed from /marketplace/products/{id} to /marketplace/products/{id}/details
Let's verify all the fields for all the apis as per this doc: https://docs.google.com/document/d/1Iw5nUpUwtofBz5pSnr7j5jXPMBlncFVdyfzLYfnN0NI/edit?tab=t.0 |
| queryKey: [filter], | ||
| }), | ||
| paginated: (params: Params = {}, filter: Filter = {}) => ({ | ||
| queryFn: () => getMarketplaceTypes(params, filter), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be getMarketplacePartners, instead of getMarketplaceTypes
| infinite: (filter: Filter = {}) => ({ | ||
| queryFn: ({ pageParam }) => | ||
| getMarketplaceProducts( | ||
| { page: pageParam as number, page_size: 25 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should page size be 25 for products api also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimum size for a page is 25
| filter: Filter, | ||
| enabled: boolean = false, | ||
| ) => { | ||
| useQuery<ResourcePage<MarketplaceCategory>, APIError[]>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This need to have a return statement before useQuery right if it's inside curly brace or remove the curly braces and use implicit return.
| filter: Filter, | ||
| enabled: boolean = false, | ||
| ) => { | ||
| useQuery<ResourcePage<MarketplacePartner>, APIError[]>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to have a return statement before useQuery
| filter: Filter, | ||
| enabled: boolean = false, | ||
| ) => { | ||
| useQuery<ResourcePage<MarketplaceProduct>, APIError[]>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to have a return statement before useQuery
| MarketplaceCategory, | ||
| MarketplacePartner, | ||
| MarketplacePartnerReferralPayload, | ||
| MarketplaceProduct, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separate types from value imports and use import type { way instead for importing the types
| placeholderData: keepPreviousData, | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no types defined in here like (like useMarketplaceTypesQuery, useAllMarketplaceTypesQuery, etc.). In keys we have queries defined for types api though. Hook should be added here for types.
| export const useMarketplaceProductsQuery = ( | ||
| params: Params, | ||
| filter: Filter, | ||
| enabled: boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we defaulting enabled to false? it means queries won't run unless explicitly enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'd prefer to keep it this way until we have the feature flag. I'm open to changing the value if needed
Cloud Manager UI test results🎉 860 passing tests on test run #3 ↗︎
|
Changes 🔄
Added queries for the following marketplace endpoints
Scope 🚢
Upon production release, changes in this PR will be visible to:
How to test 🧪
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅