-
Notifications
You must be signed in to change notification settings - Fork 584
Enforce react-hooks/exhaustive-deps lint rule #2138
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
🦋 Changeset detectedLatest commit: d643efb The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@IDubuque Can you verify that the useEffect deps changes I have made in this PR do not add unwanted side-effects? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
=======================================
Coverage 67.50% 67.50%
=======================================
Files 291 291
Lines 11017 11017
Branches 1513 1513
=======================================
Hits 7437 7437
Misses 2955 2955
Partials 625 625 ☔ View full report in Codecov by Sentry. |
waiting on @IDubuque review |
@@ -36,7 +36,7 @@ export const Modal: React.FC<{ | |||
return () => { | |||
document.removeEventListener("keydown", keyDownHandler); | |||
}; | |||
}, []); | |||
}, [escapeToClose, onClose]); |
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 one is fine
@@ -84,7 +84,7 @@ export const VerifyOwnershipWithPaper: React.FC< | |||
return () => { | |||
window.removeEventListener("message", handleMessage); | |||
}; | |||
}, []); | |||
}, [onError, onSuccess, onWindowClose]); |
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 one is fine
@@ -141,7 +141,7 @@ export const PaperCheckout = <T extends ContractType>({ | |||
return () => { | |||
window.removeEventListener("message", handleMessage); | |||
}; | |||
}, []); | |||
}, [onCloseCheckout, onPaymentSuccess, onTransferSuccess]); |
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 one is fine
appNameToUse, | ||
clientId, | ||
configs, | ||
locale, | ||
onBeforeModalOpen, | ||
onCardDetailLoad, | ||
onError, | ||
onPaymentSuccess, | ||
onPriceUpdate, | ||
onReview, | ||
options, | ||
sdkClientSecret, |
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 one needs a deeper look
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.
Changes look good
Problem solved
react-hooks/exhaustive-deps
lint rule is not followed - throw error insteadreact-hooks/exhaustive-deps
issues in payments components