-
Notifications
You must be signed in to change notification settings - Fork 149
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
Fix ecommerce-app-base bug in drag-n-drop refetch logic #7944
Fix ecommerce-app-base bug in drag-n-drop refetch logic #7944
Conversation
✅ Deploy Preview for ecommerce-app-base-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -5,53 +5,7 @@ import { ConfigAppSDK } from '@contentful/app-sdk'; | |||
|
|||
import AppConfig from './AppConfig'; | |||
import { definitions } from './parameters.spec'; | |||
|
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.
Copied/pasted this code out of AppConfig
into __mocks__
dir so that other test files can re-use this functionality.
@@ -57,28 +57,31 @@ export const SortableComponent: FC<Props> = ({ | |||
const [productPreviews, setProductPreviews] = useState<Product[]>([]); | |||
const previousSkus = usePreviousSkus(skus); | |||
|
|||
const getProductPreviews = useCallback( |
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.
removed shouldRefetch
arg from this callback and reversed the dependency, so that consumers of this callback are responsible for determining shouldRefetch
. See line 82.
This has the added benefit of removing the productPreviews
from the useCallback
dependencies, which was causing this callback to be re-defined every time productPreviews
state updated.
b1b7308
to
dbbea6f
Compare
…using infinite loop
dbbea6f
to
c64a7d9
Compare
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.
Looks good! Thank you for the helpful comments and tests in this PR 🚀
Purpose
Fix a bug that was introduced in this PR that was causing an infinite loop rerender to occur.
Root cause
usePreviousSkus()
hook is invoked to trackskus
from previous mount,fetchProductPreviews()
on mount, as expected.fetchProductPreviews()
retrieves product previews via network requestfetchProductPreviews()
updates theproductPreviews
state, causing a rerender, as expected.usePreviousSkus()
runs and retrieves the list of skus from previous mount, as well as updates the current ref to the most current list of skus (list of skus received via props).previousSkus
as a dependency, the useEffect runs again. Setting off an infinite loop.Screen capture of working solution
Approach
Testing steps
Breaking Changes
Dependencies and/or References
Deployment