UIPFU-91 - React v19: refactor away from default props for functional components #285
Conversation
This comment has been minimized.
This comment has been minimized.
| // eslint-disable-next-line no-unused-vars | ||
| onModalClose, | ||
| renderTrigger, | ||
| // eslint-disable-next-line no-unused-vars | ||
| dataKey = 'find_patron', | ||
| // eslint-disable-next-line no-unused-vars | ||
| initialSelectedUsers, |
There was a problem hiding this comment.
Why are unused variables specified?
There was a problem hiding this comment.
They are being passed to child components. Also they need default props to be mentioned.
| // don't inadvertently pass in other resources which could result in resource confusion. | ||
| const isolatedProps = _omit(this.props, ['parentResources', 'resources', 'mutator', 'parentMutator']); | ||
| } | ||
| }, [isModalOpen, afterClose]); |
There was a problem hiding this comment.
I think adding afterClose function to deps is unsafe, as it now always needs to be memorized using useCallback to avoid triggering useEffect multiple times. The previous implementation didn't require this.
There was a problem hiding this comment.
Earlier Class based approach had a provision to make callbacks after setting while setState. Functional components do not work this way, right?
Also afterClose is a prop and called only when the modal is closed and only based on some conditions, I think it is fine here.
| const Filters = ({ | ||
| activeFilters, | ||
| onChangeHandlers, | ||
| config, | ||
| resultOffset, | ||
| }) => { |
There was a problem hiding this comment.
Class components will continue to support defaultProps since there is no ES6 alternative.
Moving class components to functional ones seems redundant doc.
React v19: refactor away from default props for functional components
Is there any reason to touch class components?
There was a problem hiding this comment.
no specific reason. As we have been following the standard of creating only functional components in new developments and also refactoring the components from class based to functional based as and when there is an opportunity, I have taken this opportunity for the refactor.
|
Purpose
UIPFU-91 - React v19: refactor away from default props for functional components
Approach
refactor 4 components from class based to functional based components and refactor away from default props.
NOTE: There is no test file for UserSearchView.js
Coverage would be enhanced in the scope of an existing tech debt ticket - https://folio-org.atlassian.net/browse/UIPFU-85
Learning
Pre-Merge Checklist
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.