Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 5, 2020

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Jun 4, 2020

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.

Related

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@oatkiller oatkiller marked this pull request as ready for review June 4, 2020 19:34
@oatkiller oatkiller requested review from a team as code owners June 4, 2020 19:34
@oatkiller oatkiller requested review from a team June 4, 2020 19:34
@oatkiller oatkiller requested review from a team as code owners June 4, 2020 19:34
@oatkiller oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 4, 2020
@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team labels Jun 4, 2020
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@stacey-gammon
Copy link
Contributor

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
image

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

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Nice!

@cjcenizal
Copy link
Contributor

@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.

@@ -109,10 +109,12 @@ export const TransactionDistribution: FunctionComponent<Props> = (
bucketIndex,
} = props;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the violation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
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,
  ]);

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one clarifying question. APM changes lgtm

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ML/Transform changes LGTM

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

infra plugin changes LGTM, thanks for updating! ❤️

@oatkiller
Copy link
Contributor Author

@cjcenizal Would you be willing to approve this change?

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security LGTM. Thank you @oatkiller 💪

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM!

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@oatkiller oatkiller merged commit c98cfda into elastic:master Jun 5, 2020
@oatkiller oatkiller deleted the update-hooks-eslint-rule branch June 5, 2020 16:44
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jun 5, 2020
* 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
* updates `eslint-plugin-react-hooks` from `v2.3.0` to `v4.0.4`. 
* Disable rules on failing lines
@clintandrewhall
Copy link
Contributor

clintandrewhall commented Jun 5, 2020

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
Copy link
Contributor

sebelga commented Jun 8, 2020

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.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 8, 2020
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.