-
Notifications
You must be signed in to change notification settings - Fork 45
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
refactor: resolve hook violations #448
base: main
Are you sure you want to change the base?
Conversation
Removes the following warnings: > $ npm run lint > > [email protected] lint > next lint > > > ./src/app/login/githublogin.tsx > 32:6 Warning: React Hook useEffect has a missing dependency: 'searchParams'. Either include it or remove the dependency array. react-hooks/exhaustive-deps > > ./src/components/Contribute/Knowledge/Github/index.tsx > 429:9 Warning: The 'knowledgeFormData' object makes the dependencies of useEffect Hook (at line 449) change on every render. To fix this, wrap the initialization of 'knowledgeFormData' in its own useMemo() Hook. react-hooks/exhaustive-deps > > ./src/components/Contribute/Knowledge/Native/index.tsx > 428:9 Warning: The 'knowledgeFormData' object makes the dependencies of useEffect Hook (at line 448) change on every render. To fix this, wrap the initialization of 'knowledgeFormData' in its own useMemo() Hook. react-hooks/exhaustive-deps > > ./src/components/Contribute/Skill/Github/index.tsx > 327:9 Warning: The 'skillFormData' object makes the dependencies of useEffect Hook (at line 341) change on every render. To fix this, wrap the initialization of 'skillFormData' in its own useMemo() Hook. react-hooks/exhaustive-deps > > ./src/components/Contribute/Skill/Native/index.tsx > 304:9 Warning: The 'skillFormData' object makes the dependencies of useEffect Hook (at line 318) change on every render. To fix this, wrap the initialization of 'skillFormData' in its own useMemo() Hook. react-hooks/exhaustive-deps > > ./src/components/GithubAccessPopup/index.tsx > 28:6 Warning: React Hook React.useEffect has a missing dependency: 'onAccept'. Either include it or remove the dependency array. If 'onAccept' changes too often, find the parent component that defines it and wrap that definition in useCallback. react-hooks/exhaustive-deps > > ./src/components/PathService/PathService.tsx > 82:6 Warning: React Hook useEffect has a missing dependency: 'path'. Either include it or remove the dependency array. If 'setInputValue' needs the current value of 'path', you can also switch to useReducer instead of useState and read 'path' in the reducer. react-hooks/exhaustive-deps > 94:6 Warning: React Hook useEffect has missing dependencies: 'fetchData', 'handlePathChange', and 'validatePath'. Either include them or remove the dependency array. If 'handlePathChange' changes too often, find the parent component that defines it and wrap that definition in useCallback. react-hooks/exhaustive-deps > > info - Need to disable some ESLint rules? Learn more here: https://nextjs.org/docs/app/api-reference/config/eslint#disabling-rules Signed-off-by: Alexander Alemayhu <[email protected]>
@aalemayhu Did you get a chance to test this locally? Also can you please rebase the PR, so that i can give it a test spin in my local environment. |
@vishnoianil Yes I tried running it locally and it looked fine. I am new to instructlab so it's possible I missed steps when checking. I will rebase and check again. Thanks! |
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.
Just one minor comment with these changes.
However, I do so another issue with how validation is done by passing setDisableAction
to each of the FormGroups. Those components are setting the disable action status but it will just be overridden since it is also setting a value that is checked by the parent in the useEffect when the data changes.
Maybe not for this PR, but I think the passing of setDisabledAction
should be removed since it's not having any effect.
@@ -91,7 +94,7 @@ const PathService: React.FC<PathServiceProps> = ({ reset, rootPath, path, handle | |||
setItems([]); | |||
} | |||
validatePath(); |
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 don't think validatePath
needs to be done here. It is already done in its own effect when the inputValue
changes.
Removes the following warnings:
Signed-off-by: Alexander Alemayhu [email protected]