upgrade eslint-plugin-react-hooks from 2.3.0 to 4.0.4 by oatkiller · Pull Request #68295 · elastic/kibana

oatkiller · 2020-06-04T17:30:06Z

Summary

This PR updates eslint-plugin-react-hooks from v2.3.0 to v4.0.4. Any lint failures resulting from this are being ignored using comments.

See eslint-plugin-react-hooks's CHANGELOG.md

Background

The rules of hooks are important to follow. These can be enforced using the react-hooks eslint plugin. Kibana uses this plugin, but the version is old. Many teams have disabled the rules in their code:

The following plugins have 'exhaustive-deps' disabled:

es_ui_shared
kibana_react
kibana_utils
canvas
index_management
lens
ml
snapshot_restore
security_solution (formerly siem)

kibana_react has disabled rules_of_hooks as well.

Perhaps the latest version will address some of the issues that original drove teams to disable the rules. See the eslint-plugin-react-hooks CHANGELOG.md

The security_solution plugin would like to re-enable the rule only after upgrading to the latest major version.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-06-04T19:48:35Z

Pinging @elastic/apm-ui (Team:apm)

elasticmachine · 2020-06-04T19:48:36Z

Pinging @elastic/ingest-management (Team:Ingest Management)

stacey-gammon · 2020-06-04T19:54:57Z

Makes sense to me!

Tagging a few folks who had it turned off in their plugins. Anybody object or have a legit reason this shouldn't be turned on?

@streamich
@sebelga
@clintandrewhall

paul-tavares · 2020-06-04T20:02:46Z

x-pack/plugins/security_solution/public/endpoint_hosts/view/details/policy_response.tsx

-              id={useMemo(() => htmlIdGenerator()(), [])}
-              key={useMemo(() => htmlIdGenerator()(), [])}
+              id={
+                /* eslint-disable-next-line react-hooks/rules-of-hooks */


what is the rule here? That all hooks be called from the root of the component and its value referenced here?

Same for the other one next to it. The hook call is inside Object.entries

paul-tavares

👍
Nice!

cjcenizal · 2020-06-04T21:13:33Z

@stacey-gammon When the rules were originally added they revealed a number of violations in some of our plugins. We have issues for tracking and fixing these violations but haven't prioritized them yet.

sorenlouv · 2020-06-04T22:46:01Z

x-pack/plugins/apm/public/components/app/TransactionDetails/Distribution/index.tsx

+  /* eslint-disable-next-line react-hooks/exhaustive-deps */
  const formatYShort = useCallback(getFormatYShort(transactionType), [
    transactionType,
  ]);


What's the violation here?

My guess is that its trying to analyze the source code of the first param, but since its not a function expression, it can't.
The following is valid:

const formatYShort = useCallback(() => getFormatYShort(transactionType), [ transactionType, ]);

sorenlouv

just one clarifying question. APM changes lgtm

walterra

ML/Transform changes LGTM

weltenwort

infra plugin changes LGTM, thanks for updating! ❤️

…eslint-rule

oatkiller · 2020-06-05T14:44:08Z

@cjcenizal Would you be willing to approve this change?

patrykkopycinski

Security LGTM. Thank you @oatkiller 💪

cjcenizal

Code LGTM!

timroes

Looked at App code only. Excludes look fine for me.

kibanamachine · 2020-06-05T16:23:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ffe0e8f

History

💚 Build #51887 succeeded 44fb7ef

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* updates `eslint-plugin-react-hooks` from `v2.3.0` to `v4.0.4`. * Disable rules on failing lines

clintandrewhall · 2020-06-05T21:37:05Z

Makes sense to me!

Tagging a few folks who had it turned off in their plugins. Anybody object or have a legit reason this shouldn't be turned on?

@clintandrewhall

Thanks for the flag, @stacey-gammon ... #68453 re-enables the rule for Canvas.

sebelga · 2020-06-08T08:27:53Z

It makes sense to me!
The only reason we turned them off is that components that were created and tested without those rules, have regression (probably infinite re-renders) if we automatically let eslint add the missing dependencies.
Once I had to completely rewrite a component logic (done by another dev) to properly follow the rules.

* master: (57 commits) Add app arch team as owner of datemath package (elastic#66880) [Observability] Landing page for Observability (elastic#67467) [SIEM] Fix timeline buildGlobalQuery (elastic#68320) Optimize saved objects getScopedClient and HTTP API (elastic#68221) [Maps] Fix mb-style interpolate style rule (elastic#68413) update script to always download node (elastic#68421) [SECURITY SOLEIL] Fix selection of event type when no siem index signal created (elastic#68291) [DOCS] Adds note about configuring File Data Visualizer (elastic#68407) [DOCS] Adds link from remote clusters to index patterns (elastic#68406) [QA] slack notify on failure (elastic#68126) upgrade eslint-plugin-react-hooks from 2.3.0 to 4.0.4 (elastic#68295) moving to jira to a gold license (elastic#67178) [DOCS] Revises doc on adding data (elastic#68038) [APM] Add ThemeProvider to support dark mode (elastic#68242) Make welcome screen disabling first action in loginIfPrompted (elastic#68238) [QA] Code coverage: unskip tests, collect tests results, exclude bundles from report (elastic#64477) [ML] Functional tests - disable flaky regression and classification creation test [Alerting] change eventLog ILM requests to absolute URLs (elastic#68331) Report page load asset size (elastic#66224) [SIEM][CASE] Change SIEM to Security (elastic#68365) ...

upgrade eslint-plugin-react-hooks from 2.3.0 to 4.0.4

061f30c

oatkiller requested a review from stacey-gammon June 4, 2020 17:34

oatkiller added 2 commits June 4, 2020 14:29

Disable rules of hooks on a per line basis.

3e00900

second round of disabling rules on a per-line basis.

44fb7ef

oatkiller marked this pull request as ready for review June 4, 2020 19:34

oatkiller requested a review from a team June 4, 2020 19:34

oatkiller requested review from a team as code owners June 4, 2020 19:34

oatkiller requested review from a team June 4, 2020 19:34

oatkiller requested review from a team as code owners June 4, 2020 19:34

oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 4, 2020

kevinlog approved these changes Jun 4, 2020

View reviewed changes

botelastic bot added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. Team:Fleet Team label for Observability Data Collection Fleet team labels Jun 4, 2020

paul-tavares reviewed Jun 4, 2020

View reviewed changes

paul-tavares approved these changes Jun 4, 2020

View reviewed changes

sorenlouv reviewed Jun 4, 2020

View reviewed changes

sorenlouv approved these changes Jun 4, 2020

View reviewed changes

walterra approved these changes Jun 5, 2020

View reviewed changes

weltenwort approved these changes Jun 5, 2020

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into update-hooks-…

ffe0e8f

…eslint-rule

patrykkopycinski approved these changes Jun 5, 2020

View reviewed changes

cjcenizal approved these changes Jun 5, 2020

View reviewed changes

timroes approved these changes Jun 5, 2020

View reviewed changes

streamich approved these changes Jun 5, 2020

View reviewed changes

oatkiller merged commit c98cfda into elastic:master Jun 5, 2020

oatkiller deleted the update-hooks-eslint-rule branch June 5, 2020 16:44

oatkiller mentioned this pull request Jun 5, 2020

[7.x] upgrade eslint-plugin-react-hooks from 2.3.0 to 4.0.4 (#68295) #68410

Merged

oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jun 5, 2020

upgrade eslint-plugin-react-hooks from 2.3.0 to 4.0.4 (elastic#68295)

fd55584

* updates `eslint-plugin-react-hooks` from `v2.3.0` to `v4.0.4`. * Disable rules on failing lines

oatkiller pushed a commit that referenced this pull request Jun 5, 2020

upgrade eslint-plugin-react-hooks from 2.3.0 to 4.0.4 (#68295) (#68410)

6fdee07

* updates `eslint-plugin-react-hooks` from `v2.3.0` to `v4.0.4`. * Disable rules on failing lines

clintandrewhall mentioned this pull request Jun 5, 2020

Enable exhaustive-deps; correct any lint warnings #68453

Merged

Conversation

oatkiller commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Related

Checklist

For maintainers

Uh oh!

elasticmachine commented Jun 4, 2020

Uh oh!

elasticmachine commented Jun 4, 2020

Uh oh!

stacey-gammon commented Jun 4, 2020

Uh oh!

paul-tavares Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

oatkiller Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

paul-tavares left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Jun 4, 2020

Uh oh!

sorenlouv Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

oatkiller Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

sorenlouv left a comment

Choose a reason for hiding this comment

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

oatkiller commented Jun 5, 2020

Uh oh!

patrykkopycinski left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

timroes left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 5, 2020

💚 Build Succeeded

History

Uh oh!

clintandrewhall commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebelga commented Jun 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

oatkiller commented Jun 4, 2020 •

edited

Loading

clintandrewhall commented Jun 5, 2020 •

edited

Loading