[APM] Add react-hooks lint rules for APM folder, fix dependencies #42129
Conversation
x-pack/legacy/plugins/apm/public/components/app/Settings/AddSettings/AddSettingFlyout.tsx
Show resolved
Hide resolved
x-pack/legacy/plugins/apm/public/components/app/TransactionOverview/List/index.tsx
Show resolved
Hide resolved
💚 Build Succeeded |
| @@ -120,15 +120,19 @@ export function KueryBar() { | |||
| let didCancel = false; | |||
|
|
|||
| async function loadIndexPattern() { | |||
| setState({ ...state, isLoadingIndexPattern: true }); | |||
| setState(value => ({ ...value, isLoadingIndexPattern: true })); | |||
There was a problem hiding this comment.
... again: did the linter catch this? If so, that's great!
| preservePreviousResponse, | ||
| dispatchStatus, | ||
| ...effectKey | ||
| /* eslint-enable react-hooks/exhaustive-deps */ |
There was a problem hiding this comment.
Is it complaining about counter not being used?
There was a problem hiding this comment.
I guess that's fine. Although would be better if we could avoid disabling the lint rule somehow...
There was a problem hiding this comment.
Yeah, and we can't add fn because it's usually recreated every render. Passing fn as a dependency would be more correct, but it would mean that we require the consumer to wrap the function that is passed to useFetcher in useCallback(). Adding result.data breaks stuff as well. I figure we have to do a significant rewrite of this hook to have correct dependencies.
There was a problem hiding this comment.
Okay, let's just leave it at this, then
|
Would the lint rule catch this issue (missing const { data } = useFetcher(() => {
if (start && end) {
return loadServiceList({ start, end, uiFilters });
}
}, [start, end]);I'd be surprised if it did, since it's a custom hook. |
|
@sqren There's seemingly an additionalHooks option, we can probably use that: I'll give it a shot, and update the PR if it works. |
| @@ -0,0 +1,7 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
Can we keep the behaviour we are already doing for other overrides of keep everything from eslint in a global configuration other than have nested config files? Like for example https://github.com/elastic/kibana/blob/master/.eslintrc.js#L390,
You can just add other object with those rules on that line,
There was a problem hiding this comment.
@mistic added another item to the array that only applies to .ts(x) files (it was already applied to .js files).
|
@sqren the |
|
@dgieselaar Wow! That's super cool! |
| @@ -30,7 +30,7 @@ export function useTransactionBreakdown() { | |||
| uiFilters | |||
| }); | |||
| } | |||
| }, [serviceName, start, end, uiFilters]); | |||
| }, [serviceName, start, end, transactionType, transactionName, uiFilters]); | |||
There was a problem hiding this comment.
Did you almost forget to fix what brought you here in the first place :p
There was a problem hiding this comment.
Super nice with this custom hooks linter 👍 👍
There was a problem hiding this comment.
Yeah I actually left it open because I wanted to test it until I found out that the transaction type select was broken. And then I forgot about it 😅
There was a problem hiding this comment.
But the linter saved us! Already proving its worth! :D
💚 Build Succeeded |
…astic#42129) * [APM] Add react-hooks lint rules for APM folder, fix dependencies Closes elastic#42128. * Validate useFetcher dependencies as well * Add useFetcher hook; move eslint config to kibana eslint config
…astic#42129) * [APM] Add react-hooks lint rules for APM folder, fix dependencies Closes elastic#42128. * Validate useFetcher dependencies as well * Add useFetcher hook; move eslint config to kibana eslint config
Closes #42128.