fix(deps): update eslint and dependencies to latest and fix newly highlighted issues#8097
fix(deps): update eslint and dependencies to latest and fix newly highlighted issues#8097gribnoysup wants to merge 5 commits into
Conversation
… of potentially stale value
…id helpers in hadron packages; remove display type from element and scope the logic to editors; remove mutation of element from render
| const handleInsert = useCallback(() => { | ||
| setInsertInProgress(true); | ||
| // This is kinda silly: the banner shows up for just a blip and immediately | ||
| // disappears. There's probably a better way to deal with that state |
There was a problem hiding this comment.
Couldn't agree more with this comment. Why is updating eslint making us do this? 😅
There was a problem hiding this comment.
Previous code doing this looked like this:
useEffect(() => {
if (insertInProgress) {
setInsertInProgress(false);
}
}, [insertInProgress]);
As soon as we setInsertInProgress(true) in this handler, it will trigger a re-render and immediately setInsertInProgress(false). Using setState in effect is prohibited by the rule, so calling it inside the handler with a delay is the closest thing to what was happening before without an effect. UI-wise neither of those make much sense, doing it like that just highlights how silly it is 😅
I was going for just 1-to-1 replacement of old logic, but idk, should I just remove this in-progress code completely maybe? It doesn't really work at the moment anyway
There was a problem hiding this comment.
compass-crud/insert-document-dialog
Setting state in effect is disallowed and usually is a red flag for weird state structure
FWIW In the data viewing enhancements epic we'll be making some updates in the insert document dialog. It will involve taking a holistic view of the approach that it currently has and we'll likely do some refactoring/rewriting. So it's good not to invest too much into it now.
| return () => { | ||
| doc.addListener(Element.Events.Invalid, handleInvalid); | ||
| doc.addListener(Element.Events.Valid, handleValid); | ||
| doc.addListener(Element.Events.Removed, handleValid); |
There was a problem hiding this comment.
Are these supposed to be removeListener calls?
There was a problem hiding this comment.
Yeah, good catch. Not the first time I mistype it either, should maybe figure out a custom rule for that or something 😅
Anemy
added
the
no-title-validation
|
Seems like I missed a few in the compass-editor |
3 participants
Latest eslint update improved detection for some React related issues, this is why the autoupdate PR is failing. This patch builds on top of the autogenerated PR and fixes the issues. Every issue is resolved in its own commit:
Passing dynamic deps to memo, effect, etc is not allowed by the memo rule as this prevents React from staticaly analyzing the deps list. We're creating a hook here that should behave like useMemo so for our case it makes sense to pass this value like that. I ignored the rule with a comment. As a drive by, made sure that the getGroup function is also part of the deps list in a safe way and we always call the current one via current ref helper
Setting state in effect is diallowed and usually is a red flag for weird state structure, in this case it seems like a mix of old class component state being moved to a function component without accounting for differences between the two and just genuinely weird state structure we have there in some places. Not trying to clean this up completely, just getting rid of incorrect setState usage
The eslint error there was triggered due to mutating HadronElement inside an effect. Usually there's never a good reason for mutating a prop value directly like that, with element specifically we would always want it to be updated via explicit methods, not just mutations. For this particular case element is not even using the value in any way, so I moved it all to editor. There's probably a better way to model this in general, but I'm trying to limit the refactoring. Also removed a bunch of duplicated methods related to UUID that were spread around the files and moved them all to one place, hadron packages seem well suited to keep those helpers