Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_analyze): fix const dependency in react hooks #3508

Merged
merged 3 commits into from
Nov 2, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Oct 27, 2022

Summary

This PR extends support of useExhaustiveDependencies to consider constant captures as not needed in the dependency list.

What is a constant?

The current implementation assumes that every expression without a reference is a constant.
This bring us some polemic cases:

  • {}
  • []
  • function () {}
  • () => {}
  • class {}

These are all considered constants, even though

  • {} === {} equals false
  • [] === [] equals false
  • (function () {}) === (function () {}) equals false
  • {} === {} equals false
  • (class {}) === ( class {}) equals false

I think this is the case because JS decided to test them by reference and not by value. There are libs specially made to implement equality by value, for example: https://www.npmjs.com/package/are-equal and any assert library. I would say that equality by value is stronger than equality by reference when considering constantness.

Which makes me say that

a constant is anything whose value does not depends on values of its "environemnt".

The same cannot be said by its reference, which is implementation detail.

how eslint deal with constantness

eslint has some more complex cases for constantness that we can incorporate later.

https://eslint.org/docs/latest/rules/no-constant-condition
https://eslint.org/docs/latest/rules/no-constant-binary-expression

  • void x (always undefined)
  • x &&= false (assignment expression with a logical and with false)
  • x ||= true (assignment expression with a logical or with true)
  • Boolean(1)
  • undefined
  • someObj === {} (always false)
  • someArr === [] (always false)

Test Plan

> cargo test -p rome_js_analyze -- exhaustive 

@xunilrj xunilrj requested a review from leops as a code owner October 27, 2022 00:50
@xunilrj xunilrj requested a review from a team October 27, 2022 00:50
@netlify
Copy link

netlify bot commented Oct 27, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 6217608
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/635a6f04acbfb90008156cc2

@xunilrj xunilrj temporarily deployed to netlify-playground October 27, 2022 00:50 Inactive
@xunilrj xunilrj mentioned this pull request Oct 27, 2022
11 tasks
@github-actions
Copy link

github-actions bot commented Oct 27, 2022


pub fn is_constant(expr: &JsAnyExpression) -> bool {
for node in expr.syntax().descendants() {
if matches!(node.kind(), JsSyntaxKind::JS_REFERENCE_IDENTIFIER) {
Copy link
Contributor

@MichaReiser MichaReiser Oct 27, 2022

Choose a reason for hiding this comment

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

I like the simplicity of this but I do think it's too limited:

  • JSX gets transformed to a createElement call that, to my knowledge, returns a new object reference
  • You need to handle JsxNames: const Comp = a ? A : B; <Comp />
  • are FunctionExpression and ClassExpression const? Because they create a new instance every time the variable statement is executed. Same is true for Object or ArrayExpressions.

This question show that it is important to document the semantics of is_const. You may further consider to move this function into the specific lint rule that uses it where it's is possible to make more assumptions about the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

For JSX elements I think they can safely be considered "constant" if they only refer to other constants, at least Babel has the @babel/plugin-transform-react-constant-elements plugin to hoist these expressions out of the component render function.
For the rest while it's very important that we avoid false positives it does seem like such a simple analysis would also have a bunch of false negatives, for instance I think it would return false for const plusOne = x => x + 1 when that function could be considered constant at least for the purpose of checking React Hooks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that needs documentation, but I would leave this at the semantic model for two reasons:

  • So we have at some point a function we can use in multiple places to avoid different interpretation what is a constants;
  • In the future I would like to make this check transient and accept references to constant variables.

I incremented the summary with more possible interpretations what a constant is.

@xunilrj xunilrj temporarily deployed to netlify-playground October 27, 2022 11:44 Inactive
@xunilrj xunilrj added this to the 10.0.0 milestone Nov 2, 2022
@xunilrj xunilrj merged commit 5745b7a into main Nov 2, 2022
@xunilrj xunilrj deleted the fix/useExhaustiveDependencies-better-const branch November 2, 2022 10:38
@ematipico ematipico mentioned this pull request Nov 15, 2022
29 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants