ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578881574
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
I think this is the style we prefer:
```suggestion
export default function FlashProvide({ children, messages }:
FlashProvideProps) {
```
1. Use named function instead of arrow functions, make it easier to find in
debugging code
2. Prefix props with component name, make it less likely to have export
conflicts
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
@yardz Please refer to this post for why ignoring the exhaustive-deps
warning is a bad idea:
https://kentcdodds.com/blog/react-hooks-pitfalls#pitfall-2-not-using-or-ignoring-the-eslint-plugin
In my experience, most places where `useEffect` with an empty dependency
list are used, it's either very easy to add the exhaustive deps without adding
unnecessary re-renderings, or more correct to do so:
- `DynamicPlugins` : ESLint is not reporting error
- `useWindowSize`: it's more accurate to add `delayMs` because if you pass a
different `delayMs`, it should add a new event handler (and remove the previous
one).
- `ExploreViewContainer`: dependencies should be added as recommended,
handler functions should be wrapped with `useCallback`. Because when the
component get re-rendered, the old handlers would refer to closured variables
in previously rendering cycle.
- `useListViewResource`: dependencies should be added because if the
component that calls the hook re-renders and requests a different resource or
passes in a different handler, there will be no API requests made. This makes
the hook functionally incorrect. The correct way to avoid excessive requests is
to cache API requests in another layer (either another hook or a new API client
with cache support).
- `Welcome.tsx`: the only real dependency is `user_id`, adding it will not
cause excessive re-rendering, but will also make it possible to re-render the
page for different users (without unmounting the component).
Fixing the lint error is also very easy in most cases, you can simply hit
"Cmd + ." in VSCode and use the recommended fix:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
