Enable ESlint rules for React Hooks #6651
Conversation
|
Turns out we hade quite few hook violations... I'll fix them after #6263 is merged as they have a lot of overlap in the files they change. |
VIKTORVAV99
added
the
techdebt
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for taking on this one 💚
Looks really good, couple of small fixes:
- Solar layer can get stuck on
- The line on the charts that previous followed the selectedDatetime is now missing
This was quite a big PR 😅, could have been nice to isolate the hooks fixes from other fixes but overall very nice 🙌
| @@ -93,7 +93,7 @@ export default function SolarLayer({ map }: { map?: maplibregl.Map }) { | |||
| map.removeSource('solar'); | |||
| } | |||
| }; | |||
| }, [map, node, isVisibleReference.current]); | |||
There was a problem hiding this comment.
I believe without this reference the solar lay can get stuck on
There was a problem hiding this comment.
I have added it back but ESlint throws a warning now:
React Hook useEffect has an unnecessary dependency: 'isVisibleReference.current'. Either exclude it or remove the dependency array. Mutable values like 'isVisibleReference.current' aren't valid dependencies because mutating them doesn't re-render the component.
However since it's just a warning we can adress it later.
They are all hook fixes (if we include the dependency arrays) 😅 Fix one thing and it complains about the next, it's all pretty simple but deep issues though. I'll take a look at the solar layer and selectedDatetime though. |
| @@ -93,7 +93,7 @@ | |||
| map.removeSource('solar'); | |||
| } | |||
| }; | |||
| }, [map, node, isVisibleReference.current]); | |||
| }, [map, node, setIsLoadingSolarLayer, isVisibleReference.current]); | |||
Check warning
Code scanning / ESLint
verifies the list of dependencies for Hooks like useEffect and similar Warning
There was a problem hiding this comment.
Solar layer don't appears to be working correctly without it. We should address that in another PR.
|
I can't really figure out how to make the Estimation cards do what we want with these new restrictions without significant changes. We could restore them and skip the changes in here, ESlint would error in this PR but let other PRs pass in the future. We would have to change how earthly works though as it also run ESLint for some reason? |
Could we address the fixes that don't require significant changes then turn the ESLint rule off until we want to address them? |
We could, but then we run the risk of reintroducing some of them, but might be worth it anyway? I would keep all the changes except for the estimation card stuff if we were to do this. |
It's a nice improvement even without the rule enabled. Yeah I think that's a logical split on what to include. Is it possible to exclude a line or file from the linting? |
|
It is, so we could also do that, which is probably preferable. |
|
Oh god now this PR have the same issue as #6697 ... I'll try and fix it in that PR directly then we can hopefully merge this. |
|
Let me know when this needs review again 🚀 |
It does! (unless you want me to merge it without one 😉) |
|
Kudos for getting this one through and making the codebase a better place ❤️ |
Issue
Closes: AVO-172
Description
Preview
Double check
poetry run test_parser "zone_key"pnpx prettier@2 --write .andpoetry run formatin the top level directory to format my changes.