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

fix(rome_js_analyze): improve the detection of ReactDOM.render calls in noRenderReturnValue #3626

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Nov 9, 2022

Summary

Fixes #3617

This refactors the noRenderReturnValue rule to use is_react_call_api to detect calls to ReactDOM.render. This includes extending is_react_call_api to support detecting symbols imported from react-dom in addition to the react package.

Test Plan

I've extended the tests for the noRenderReturnValue to have cases with and without an import statement, and check for regressions of the false positive case described in #3617

@leops leops requested a review from xunilrj as a code owner November 9, 2022 16:36
@netlify
Copy link

netlify bot commented Nov 9, 2022

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit e1039ae
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/636be03d349bdf0008ae580c

@leops leops temporarily deployed to netlify-playground November 9, 2022 16:36 Inactive
@leops leops added this to the 10.0.1 milestone Nov 9, 2022
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I may have missed it. Did you add a test for the case where the user defines a custom render function?

crates/rome_js_analyze/src/react.rs Outdated Show resolved Hide resolved
@leops leops temporarily deployed to netlify-playground November 9, 2022 17:09 Inactive
@leops leops force-pushed the fix/no-render-return-value branch from 14e7c29 to e1039ae Compare November 9, 2022 17:15
@leops leops temporarily deployed to netlify-playground November 9, 2022 17:15 Inactive
@leops
Copy link
Contributor Author

leops commented Nov 9, 2022

I may have missed it. Did you add a test for the case where the user defines a custom render function?

Good catch, I did add it originally but it got lost when I split out the single test files into multiple modules

@leops leops merged commit 3b88794 into main Nov 10, 2022
@leops leops deleted the fix/no-render-return-value branch November 10, 2022 08:23
jeysal added a commit to jeysal/rometools that referenced this pull request Nov 10, 2022
* upstream/main:
  fix(npm/js-api): Import type from @rometools/backend-jsonrpc (rome#3647)
  doc(npm/js-api): Add experimental warning to README
  fix(npm/js-api): Lazy load backend implementations (rome#3652)
  feat(rome_lsp): stop the daemon after the last client disconnects (rome#3643)
  fix(npm/js_api): Ensure JS API build contains `dist` folder (rome#3648)
  fix(rome_cli): ensures the service only connects to compatible versions (rome#3642)
  feat(playground): add settings button and group IR (rome#3559)
  release: v10.0.1 (rome#3649)
  fix(rome_js_analyze): False positives for interactive elements in `useKeyWithClickEvents` (rome#3644)
  fix(rome_js_semantic): support for object and array bindings when exporting (rome#3620)
  doc(editors): Clarify Rome discovery (rome#3639)
  fix(rome_js_analyze): improve the detection of `ReactDOM.render` calls in `noRenderReturnValue` (rome#3626)
  chore(npm): add license (rome#3629)
  Add rustdocs index
  Add environment
  Change rust docs to use Netlify
  fix(rome_js_analyze): useValidAnchor considering all possible expressions (rome#3599)
  fix(rome_js_formatter): Trailing comma inside import rome#3600 (rome#3624)
@leops leops added the A-Linter Area: linter label Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 noRenderReturnValue incorrectly flags non React render calls
2 participants