-
Notifications
You must be signed in to change notification settings - Fork 1
feat(projects): add organizationId and includeMembers parameters to useProjects hook #607
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
Conversation
…seProjects hook - Add optional organizationId parameter for filtering projects by organization - Add optional includeMembers parameter (defaults to true, breaking change from false) - Implement parameter-based caching with unique cache keys - BREAKING CHANGE: useProjects() now returns projects with members by default to match the Sanity Client
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
binoy14
left a comment
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.
Comment regarding the default but looks good otherwise
| }).observable.pipe( | ||
| switchMap((client) => client.observable.projects.list({includeMembers: false})), | ||
| switchMap((client) => { | ||
| const includeMembers = options?.includeMembers ?? true |
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.
I think we should default to not including members, it can be very slow and we call this on the mount of the SDK, it's also a breaking expectations change which is not really worth changing IMO.
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.
I switched this back to defaulting to false
| }, | ||
| fetcher: (instance) => (options?: {organizationId?: string; includeMembers?: boolean}) => | ||
| getClientState(instance, { | ||
| apiVersion: API_VERSION, |
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.
Not really part of your PR but since you are here could you also tag the requests so it's more explicit and not sanity.sdk generic
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.
Added
|
Ready for another review |
binoy14
left a comment
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.
makes sense to me, thanks
…seProjects hook (#607) - Add optional organizationId parameter for filtering projects by organization - Add optional includeMembers parameter - Implement parameter-based caching with unique cache keys
Description
What to review
The changes to the projects fetcher store in
projects.tswhich now supports filtering by organization and toggling member inclusion. The cache key generation now incorporates these parameters to ensure proper caching behavior.The updated type definitions in
useProjects.tsthat reflect the new parameter options and return types.Testing
Added test coverage for the parameter passing behavior. The test in
projects.test.tsnow verifies that the correct parameters are passed to the list method.Fun gif