refactor: [M3-7897] - Query Key Factory for Regions - #10301
bnussman-akamai merged 8 commits into
Conversation
bnussman-akamai
added
the
React Query
|
Coverage Report: ✅ |
bnussman-akamai
changed the title
hkhalil-akamai
left a comment
hkhalil-akamai
left a comment
There was a problem hiding this comment.
Feeling confident about these changes after some manual testing and all automated tests passing. Great work on continuing the query key factory work @bnussman-akamai!
Left some comments related to file structure/organization.
| @@ -15,5 +16,6 @@ import { profileQueries } from './profile'; | |||
| export const queries = mergeQueryKeys( | |||
There was a problem hiding this comment.
I feel like we should have one consistent way to access the query keys. Most of the app directly imports each factory from its respective file, but it seems like this export has only one usage.
I prefer the first way, and if we stick to that, maybe we can delete this index file (and the need to keep it up to date) altogether.
Sorry, I realize I should've made this comment a few PRs ago.
There was a problem hiding this comment.
Yeah, I think I'm cool with deleting the index file and just doing the direct imports!
There was a problem hiding this comment.
Do we have a convention on when to break out these requests into a separate requests.ts file? For most queries, it seems like the hooks and request helpers live together in one file. I'm fine either way but just curious.
There was a problem hiding this comment.
Definitely could go either way but maintaining the split between the two files does feel a bit cleaner
There was a problem hiding this comment.
There's no convention, but I think having a dedicated requests.ts helps clean up the file that contains the hooks. I also ran into some circular dependency issues when doing the query key factory for account related endpoints, and I found that having a dedicated requests.ts made sense to help that too
dwiley-akamai
left a comment
dwiley-akamai
left a comment
There was a problem hiding this comment.
Region-related functionality throughout app ✅
Data in RQ DevTool ✅
| enabled, | ||
| }); | ||
|
|
||
| export const useRegionsAvailabilityQuery = ( |
There was a problem hiding this comment.
I think it'd be clearer to call this useRegionAvailabilityQuery (since it's for a single region) 🤔
There was a problem hiding this comment.
Definitely could go either way but maintaining the split between the two files does feel a bit cleaner
4 participants
Description 📝
Uses our new query key factory for region queries 🗺️
How to test 🧪
As an Author I have considered 🤔