fix: reduce false positives #2434
Conversation
…xternalised dependencies
…by tracking imports
github-project-automation
bot
moved this to 📬 Triage
in Plugins Platform / Grafana Community
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce false positives in react-detect by (1) tracking whether flagged identifiers are actually imported from react / react-dom, and (2) filtering out issues coming from Grafana-externalized dependencies.
Changes:
- Added
trackImportsFromPackage()AST utility to collect default/named imports (and CommonJSrequire) from a specific package. - Updated several pattern matchers to consult tracked imports when detecting
findDOMNode,ReactDOM.render,unmountComponentAtNode, andcreateFactory, plus expanded matcher test coverage. - Filtered out dependency matches whose (root) dependency is Grafana-externalized, plus added results tests for the filtering behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-detect/src/utils/ast.ts | Adds import/require tracking helper for a given package. |
| packages/react-detect/src/patterns/matcher.ts | Uses import tracking to reduce false positives for several patterns. |
| packages/react-detect/src/patterns/matcher.test.ts | Adds test cases for import-aware matching behavior. |
| packages/react-detect/src/results.ts | Filters out externalized dependency matches before reporting. |
| packages/react-detect/src/results.test.ts | Adds tests validating externalized dependency filtering. |
| if (match.type === 'dependency' && match.rootDependency && isExternal(match.rootDependency)) { | ||
| return false; |
There was a problem hiding this comment.
Externalized dependency filtering currently only triggers when match.rootDependency is set. When dependency analysis is skipped or dependency loading fails (depContext null), rootDependency is typically undefined, so direct externalized deps (e.g. react, lodash, @grafana/*) will no longer be filtered and false positives will return. Consider falling back to match.packageName (or match.rootDependency ?? match.packageName) when checking isExternal, and add a test for the depContext/rootDependency-missing scenario.
| if (match.type === 'dependency' && match.rootDependency && isExternal(match.rootDependency)) { | |
| return false; | |
| if (match.type === 'dependency') { | |
| const depName = match.rootDependency ?? match.packageName; | |
| if (depName && isExternal(depName)) { | |
| return false; | |
| } |
| node.callee.property.name === 'findDOMNode' | ||
| ) { | ||
| const objectName = node.callee.object.name; | ||
| if (imports.defaultImports.has(objectName) || objectName === 'ReactDOM') { |
There was a problem hiding this comment.
These matchers still accept objectName === 'ReactDOM' even when there is no import/require from 'react-dom' in the file. That reintroduces the same class of false positives this PR is trying to eliminate (any local variable/object named ReactDOM with a .render/.findDOMNode property will match). Consider only matching MemberExpression calls when the object identifier is known to be imported/required from 'react-dom' (including default/namespace/require), rather than special-casing the literal name 'ReactDOM'.
| if (imports.defaultImports.has(objectName) || objectName === 'ReactDOM') { | |
| if (imports.defaultImports.has(objectName)) { |
| node.callee.property.name === 'createFactory' && | ||
| hasValidArgs | ||
| ) { | ||
| if (imports.defaultImports.has(node.callee.object.name) || node.callee.object.name === 'React') { |
There was a problem hiding this comment.
Similarly, allowing node.callee.object.name === 'React' without verifying an import/require from 'react' can match unrelated local identifiers named React and cause false positives. To keep the new import-tracking approach consistent, consider requiring the object identifier to be one of the tracked imports (default/namespace/require) before reporting createFactory usage.
| if (imports.defaultImports.has(node.callee.object.name) || node.callee.object.name === 'React') { | |
| const calleeObjectName = node.callee.object.name; | |
| if ( | |
| imports.defaultImports.has(calleeObjectName) || | |
| imports.namespaceImports.has(calleeObjectName) || | |
| imports.requireImports.has(calleeObjectName) | |
| ) { |
| export function findFindDOMNode(ast: TSESTree.Program, code: string): PatternMatch[] { | ||
| const matches: PatternMatch[] = []; | ||
| const imports = trackImportsFromPackage(ast, 'react-dom'); | ||
| const findDOMNodeLocalNames = imports.namedImports.get('findDOMNode') || new Set(); | ||
|
|
There was a problem hiding this comment.
Each of these matchers calls trackImportsFromPackage (full AST walk) and then does another full walk to find call sites. Since findPatternMatches invokes many matchers per file, this adds multiple extra traversals per file. Consider computing import tracking once per file (e.g., in findPatternMatches) and passing it into the relevant matchers, or memoizing trackImportsFromPackage by (ast, packageName), to reduce analysis time on large bundles.
grafana-plugins-platform-bot
bot
moved this from 📬 Triage
to 🔬 In review
in Plugins Platform / Grafana Community
hugohaggmark
left a comment
hugohaggmark
left a comment
There was a problem hiding this comment.
Great with all the tests 🚀
| node.callee && | ||
| node.callee.type === 'MemberExpression' && | ||
| node.callee.object && | ||
| node.callee.object.type === 'Identifier' && | ||
| node.callee.property && | ||
| node.callee.property.type === 'Identifier' && | ||
| node.callee.property.name === 'findDOMNode' |
There was a problem hiding this comment.
nit: maybe this could be simplified?
| node.callee && | |
| node.callee.type === 'MemberExpression' && | |
| node.callee.object && | |
| node.callee.object.type === 'Identifier' && | |
| node.callee.property && | |
| node.callee.property.type === 'Identifier' && | |
| node.callee.property.name === 'findDOMNode' | |
| node.callee?.type === 'MemberExpression' && | |
| node.callee.object?.type === 'Identifier' && | |
| node.callee.property?.type === 'Identifier' && | |
| node.callee.property?.name === 'findDOMNode' |
| if (node.callee && node.callee.type === 'Identifier' && findDOMNodeLocalNames.has(node.callee.name)) { | ||
| matches.push(createPatternMatch(node, 'findDOMNode', code)); | ||
| } |
There was a problem hiding this comment.
nit: maybe this could be simplified?
| if (node.callee && node.callee.type === 'Identifier' && findDOMNodeLocalNames.has(node.callee.name)) { | |
| matches.push(createPatternMatch(node, 'findDOMNode', code)); | |
| } | |
| if (node.callee?.type === 'Identifier' && findDOMNodeLocalNames.has(node.callee.name)) { | |
| matches.push(createPatternMatch(node, 'findDOMNode', code)); | |
| } |
| node.callee && | ||
| node.callee.type === 'MemberExpression' && | ||
| node.callee.object && | ||
| node.callee.object.type === 'Identifier' && | ||
| node.callee.property && | ||
| node.callee.property.type === 'Identifier' && | ||
| node.callee.property.name === 'render' && |
There was a problem hiding this comment.
nit: same as previous comments
| node.callee && | ||
| node.callee.type === 'MemberExpression' && | ||
| node.callee.object && | ||
| node.callee.object.type === 'Identifier' && | ||
| node.callee.property && | ||
| node.callee.property.type === 'Identifier' && | ||
| node.callee.property.name === 'unmountComponentAtNode' && |
There was a problem hiding this comment.
nit: same as previous comments
| node.callee && | ||
| node.callee.type === 'MemberExpression' && | ||
| node.callee.object && | ||
| node.callee.object.type === 'Identifier' && | ||
| node.callee.property && | ||
| node.callee.property.type === 'Identifier' && | ||
| node.callee.property.name === 'createFactory' && | ||
| hasValidArgs | ||
| ) { |
There was a problem hiding this comment.
nit: same as previous comments
| * // For code: import { render as r } from 'react-dom' | ||
| * // Returns: { defaultImports: Set(), namedImports: Map([['render', Set(['r'])]]) } | ||
| */ | ||
| export function trackImportsFromPackage(ast: TSESTree.Program, packageName: string): ImportTracking { |
There was a problem hiding this comment.
nit: this function seems a bit overly complex and some new functions could potentially be extracted from it
There was a problem hiding this comment.
Yeah, it's not the nicest and I'm probably going too fast here. Would splitting it into functions for dealing with es6 import, cjs require and named imports be helpful? Maybe you had other ideas?
There was a problem hiding this comment.
no I think that would do nicely, but these are just nits, no real need todo anything about it
There was a problem hiding this comment.
I'll make the change as I think it fair we hold the door open for the next engineer that has to venture down this (dark) path. 🐉
2 participants
What this PR does / why we need it:
Whilst going through the list of troublesome dependencies and checking their source code to confirm if they're react 18 & 19 compatible I discovered a lot of false positives mostly due to matching function names like
renderorfindDOMNode.We've also had a report of issues being raised by the react-detect cli for externalised dependencies which feels wrong as most plugins will use these externalised dependencies but none of the code is bundled.
Originally I'd planned to make this lib as naive as possible but having thought some more on it I think the solution I've got here to track imports from packages (e.g.
react/react-dom) in the source files and then walk the AST checking if the Identifier name is listed in the imports should work. You can't importfindDOMNodeand then have a function with the same name in a js module, right?For any deps that are externalised (either directly or indirectly due to their parent deps) I'm now filtering them out.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
📦 Published PR as canary version:
Canary Versions✨ Test out this PR locally via:
npm install @grafana/create-plugin@6.8.4-canary.2434.21668000529.0 npm install @grafana/plugin-docs-renderer@0.0.2-canary.2434.21668000529.0 npm install @grafana/react-detect@0.5.2-canary.2434.21668000529.0 # or yarn add @grafana/create-plugin@6.8.4-canary.2434.21668000529.0 yarn add @grafana/plugin-docs-renderer@0.0.2-canary.2434.21668000529.0 yarn add @grafana/react-detect@0.5.2-canary.2434.21668000529.0