Implement SWR into hooks #701
Conversation
❌ Deploy Preview for commercelayer-react-components failed.
|
There was a problem hiding this comment.
Pull request overview
This PR implements SWR (stale-while-revalidate) caching into the usePrices hook and adds support for retrieving and updating individual prices. The implementation also updates various dependencies across the monorepo packages.
Changes:
- Integrated SWR library into the
usePriceshook for data caching and revalidation - Added
retrievePriceandupdatePricefunctions to the hook's API - Updated multiple package dependencies to their latest versions
- Updated biome.json schema and formatting configuration
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/hooks/src/prices/usePrices.ts | Refactored to use SWR for caching, added retrieve and update operations |
| packages/hooks/src/prices/usePrices.test.ts | Added tests for new retrieve and update functionality |
| packages/hooks/src/index.ts | Created index file exporting usePrices hook |
| packages/hooks/package.json | Added swr dependency and updated dev dependencies |
| packages/core/src/prices/index.ts | Exported PriceUpdate type |
| packages/react-components/package.json | Updated dependencies |
| packages/document/package.json | Updated dependencies |
| packages/docs/package.json | Updated dependencies |
| packages/core/package.json | Updated dependencies |
| package.json | Updated biome dependency |
| biome.json | Updated schema version and reformatted |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| coreTest("should update a price", async ({ accessToken }) => { | ||
| const token = accessToken?.accessToken | ||
| const { result } = renderHook(() => usePrices(token)) | ||
|
|
||
| // First fetch prices | ||
| act(() => { | ||
| result.current.fetchPrices() | ||
| }) | ||
|
|
||
| await waitFor(() => { | ||
| expect(result.current.prices.length).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| const priceToUpdate = result.current.prices[0] | ||
|
|
||
| // Update the price | ||
| await act(async () => { | ||
| await result.current.updatePrice({ | ||
| id: priceToUpdate.id, | ||
| type: "prices", | ||
| }) | ||
| }) | ||
|
|
||
| await waitFor(() => { | ||
| expect(result.current.action).toBe("update") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
This test only verifies that the action state is set to "update", but doesn't verify that the price was actually updated, that the update was reflected in the prices array, or that the return value from updatePrice contains the correct updated data. The test is incomplete and doesn't validate the core functionality.
The test should verify that the returned promise resolves to the updated price and that result.current.prices reflects the update.
| mutate(data, false) | ||
| }, [mutate, data]) |
There was a problem hiding this comment.
The clearError callback includes data in its dependency array, causing it to be recreated every time the data changes. This is a performance concern and can cause unnecessary re-renders in consuming components.
Consider using a ref to track the data or restructuring this callback to avoid the data dependency.
| mutate(data, false) | |
| }, [mutate, data]) | |
| mutate((currentData) => currentData, false) | |
| }, [mutate]) |
| const handleUpdatePrice = useCallback( | ||
| async (resource: PriceUpdate): Promise<Price | undefined> => { | ||
| setPriceResource(resource) | ||
| setAction("update") | ||
| setShouldFetch(true) | ||
| await mutate() | ||
| return data?.find((p: Price) => p.id === resource.id) | ||
| }, | ||
| [accessToken], | ||
| [mutate, data], | ||
| ) |
There was a problem hiding this comment.
The handleUpdatePrice function has the same race condition as handleRetrievePrice. It sets state variables and then immediately calls mutate() and returns data?.find(...). The state updates are asynchronous, so the SWR cache key may not have updated when mutate() is called, and the returned data may be stale.
Additionally, the function searches for the updated price in the current data array, but this search happens before the update has actually been fetched and applied to the SWR cache.
| return await getPrices({ | ||
| accessToken, | ||
| params, | ||
| }) |
There was a problem hiding this comment.
The SWR fetcher function has a default case that duplicates the "get" case logic. This is redundant since the action state should always be one of the expected values ("get", "retrieve", "update", or null). If action is null, the SWR key would be null and the fetcher shouldn't be called.
The default case should either throw an error for unexpected action values or be removed entirely.
| return await getPrices({ | |
| accessToken, | |
| params, | |
| }) | |
| throw new Error(`Unexpected action: ${String(action)}`) |
| const handleUpdatePrice = useCallback( | ||
| async (resource: PriceUpdate): Promise<Price | undefined> => { | ||
| setPriceResource(resource) | ||
| setAction("update") | ||
| setShouldFetch(true) | ||
| await mutate() | ||
| return data?.find((p: Price) => p.id === resource.id) | ||
| }, | ||
| [accessToken], | ||
| [mutate, data], | ||
| ) |
There was a problem hiding this comment.
The handleUpdatePrice callback includes data in its dependency array, which will cause the callback reference to change every time the data updates. This is unnecessary since the function only needs the current value of data when it executes, not when it's created.
This can lead to performance issues and unexpected behavior in components that use this callback as a dependency.
| const fetchPrices = useCallback( | ||
| (newParams?: Parameters<typeof getPrices>[0]["params"]) => { | ||
| setParams(newParams) | ||
| setShouldFetch(true) | ||
| setAction("get") | ||
| }, | ||
| [], | ||
| ) |
There was a problem hiding this comment.
The fetchPrices function always sets shouldFetch to true, but never sets it back to false after the fetch completes. This means once fetchPrices is called, shouldFetch remains true indefinitely, which could cause unexpected revalidations or side effects if the component re-renders and the SWR key changes.
Consider adding logic to reset shouldFetch after the fetch completes, or reconsider whether this state variable is necessary for the design.
| const { data, error, isLoading, isValidating, mutate } = useSWR<Price[]>( | ||
| shouldFetch && accessToken && action | ||
| ? ["prices", action, accessToken, params, priceId, priceResource] | ||
| : null, | ||
| async (): Promise<Price[]> => { | ||
| switch (action) { | ||
| case "get": | ||
| return await getPrices({ | ||
| accessToken, | ||
| params, | ||
| }) | ||
| setState((prev: UsePricesState) => ({ | ||
| ...prev, | ||
| prices: prices, | ||
| })) | ||
| } catch (error: unknown) { | ||
| setState((prev: UsePricesState) => ({ | ||
| ...prev, | ||
| error: | ||
| error instanceof Error ? error.message : "Failed to fetch prices", | ||
| })) | ||
| case "retrieve": { | ||
| if (!priceId) throw new Error("Price ID is required for retrieve") | ||
| return [await retrievePrice({ accessToken, id: priceId })] | ||
| } | ||
| }) | ||
| case "update": { | ||
| if (!priceResource) | ||
| throw new Error("Price resource is required for update") | ||
| const updatedPrice = await updatePrice({ | ||
| accessToken, | ||
| resource: priceResource, | ||
| }) | ||
| return data | ||
| ? data.map((p: Price) => | ||
| p.id === updatedPrice.id ? updatedPrice : p, | ||
| ) | ||
| : [updatedPrice] | ||
| } | ||
| default: | ||
| return await getPrices({ | ||
| accessToken, | ||
| params, | ||
| }) | ||
| } | ||
| }, | ||
| { | ||
| revalidateOnFocus: false, | ||
| revalidateOnReconnect: false, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Using a single SWR instance to handle three different operations (get list, retrieve single, update single) with different data structures is an anti-pattern. SWR is designed to cache and revalidate a specific resource at a specific URL/key.
The current implementation tries to use SWR for imperative operations (retrievePrice, updatePrice) which should happen on-demand, not as part of a cache/revalidation strategy. The retrieve and update operations should either:
- Be implemented without SWR (direct API calls)
- Use separate SWR hooks with appropriate keys
- Use SWR's mutation functionality (useSWRMutation) for updates
The current approach creates confusion about what data is cached and when, and makes the caching strategy unpredictable.
| const handleRetrievePrice = useCallback( | ||
| async (id: string): Promise<Price | undefined> => { | ||
| setPriceId(id) | ||
| setAction("retrieve") | ||
| setShouldFetch(true) | ||
| // Wait for SWR to fetch | ||
| await mutate() | ||
| return data?.[0] | ||
| }, | ||
| [mutate, data], | ||
| ) |
There was a problem hiding this comment.
The handleRetrievePrice function has a race condition. It sets state variables (setPriceId, setAction, setShouldFetch) and then immediately calls mutate() and returns data?.[0]. However, the state updates are asynchronous and may not have taken effect when mutate() is called. Additionally, even after mutate() completes, the returned value data?.[0] may still be the old data from before the state changes triggered a new SWR fetch.
The function should wait for the SWR refetch to complete before returning the data. Consider using the return value from mutate() or implementing a different pattern to ensure the correct data is returned.
| const { data, error, isLoading, isValidating, mutate } = useSWR<Price[]>( | ||
| shouldFetch && accessToken && action | ||
| ? ["prices", action, accessToken, params, priceId, priceResource] | ||
| : null, |
There was a problem hiding this comment.
The SWR cache key includes all state variables (params, priceId, priceResource), which means every state change creates a new cache entry. This defeats the purpose of caching and can lead to memory leaks as the cache grows unbounded. For example, calling retrievePrice with different IDs will create separate cache entries for each ID, and the old entries won't be reused.
Consider using a simpler cache key (e.g., just the accessToken) and managing the data transformations separately, or use SWR's mutate function with optimistic updates.
| const updatedPrice = await updatePrice({ | ||
| accessToken, | ||
| resource: priceResource, | ||
| }) | ||
| return data | ||
| ? data.map((p: Price) => | ||
| p.id === updatedPrice.id ? updatedPrice : p, | ||
| ) | ||
| : [updatedPrice] |
There was a problem hiding this comment.
The update action attempts to merge the updated price with the existing data array, but this logic is flawed. The data variable referenced here is the previous SWR data state, not the result of the update operation. If data is undefined or doesn't contain the price being updated, this will return an array with just the updated price, potentially losing other prices that were previously loaded.
This also creates an inconsistent behavior where the update operation changes the entire prices array structure based on whether there was previous data or not.
| const updatedPrice = await updatePrice({ | |
| accessToken, | |
| resource: priceResource, | |
| }) | |
| return data | |
| ? data.map((p: Price) => | |
| p.id === updatedPrice.id ? updatedPrice : p, | |
| ) | |
| : [updatedPrice] | |
| await updatePrice({ | |
| accessToken, | |
| resource: priceResource, | |
| }) | |
| // After updating a price, refetch the prices list to ensure | |
| // the cached data remains consistent with the backend. | |
| return await getPrices({ | |
| accessToken, | |
| params, | |
| }) |
showLoaderinPaymentMethodcomponentshowLoaderflow