feat(lint): add Flow.Node render rule#618
Conversation
commit: |
Docs PreviewCommit: |
Visual Regression Report — 26 changed, 15 unchanged26 screenshot(s) with visual changes: Button / Variant: Primary473 px (0.47%) changed
Button / Variant: Destructive1,030 px (1.01%) changed
Button / Variant: Outline269 px (0.26%) changed
Button / Variant: Secondary Destructive1,122 px (1.11%) changed
Button / Sizes1,507 px (1.48%) changed
Button / Loading State518 px (0.51%) changed
Button / Disabled State117 px (0.12%) changed
Button / Link as Button820 px (0.81%) changed
Dialog / Dialog With Actions298 px (0.29%) changed
Dialog / Dialog Alert1,635 px (1.61%) changed
Dialog / Dialog Confirmation1,226 px (1.21%) changed
Dialog / Custom Max Width875 px (0.86%) changed
Dialog / Dialog With Combobox480 px (0.47%) changed
Dialog / Dialog With Dropdown383 px (0.38%) changed
Select / Select Basic1,097 px (1.08%) changed
Select / Select Without Label723 px (0.71%) changed
Select / Select With Description2,199 px (1.86%) changed
Select / Select With Error1,927 px (1.63%) changed
Select / Select Placeholder1,308 px (1.29%) changed
Select / Select With Tooltip1,062 px (1.05%) changed
Select / Select Loading2,375 px (1.18%) changed
Select / Select Complex1,279 px (1.08%) changed
Select / Select Disabled Options1,217 px (1.2%) changed
Select / Select Disabled Items793 px (0.78%) changed
Select / Select Grouped With Disabled1,189 px (1.17%) changed
Select (Open)2,509 px (0.01%) changed
15 screenshot(s) unchanged
Generated by Kumo Visual Regression |
|
/bonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall looks good — a well-structured rule that directly maps to Flow.Node's cloneElement passthrough requirements.
A few notes:
-
expressionContainsIdentifierfalse positive: it currently matches identifiers used asMemberExpressionproperty names. If a developer names the ref paramrefand writesref={callbacks.ref}, the rule incorrectly counts that as forwarding the ref. Consider passing parent context throughtraverseso you can skip non-computed member properties. -
Missing tests for edge cases: default destructured params (
({ ...rest } = {})), HOC-wrappedforwardRef(memo(forwardRef(...))), and aliased imports (import { forwardRef as fr }) are all supported by the implementation but untested. -
Duplication: The rule file is duplicated between
lint/andpackages/kumo/lint/. I realize this is required becausepackages/kumo-docs-astro/.oxlintrc.jsonloads the root plugin whilepackages/kumo/.oxlintrc.jsonloads the package plugin, so it's consistent with the existing setup. -
Configs: Root, package, and docs configs all correctly enable the rule. The changeset is appropriately scoped to
@cloudflare/kumo.
| function expressionContainsIdentifier(node, names) { | ||
| let found = false; | ||
| traverse(node, { | ||
| Identifier(identifier) { |
There was a problem hiding this comment.
The expressionContainsIdentifier helper matches identifiers in any position, including MemberExpression property names. If a component names the ref param ref and writes ref={callbacks.ref}, the rule incorrectly counts this as forwarding the ref.
Consider passing parent context through traverse so you can skip non-computed member properties, or do a targeted walk instead of the generic one here.
| function getPropsParamNames(param) { | ||
| const names = new Set(); | ||
| const unwrapped = unwrapExpression(param); | ||
|
|
||
| if (unwrapped?.type === "Identifier") { | ||
| names.add(unwrapped.name); | ||
| } else if (unwrapped?.type === "AssignmentPattern") { | ||
| for (const name of getPropsParamNames(unwrapped.left)) { | ||
| names.add(name); | ||
| } | ||
| } else if (unwrapped?.type === "ObjectPattern") { | ||
| for (const prop of unwrapped.properties ?? []) { | ||
| if (prop.type === "RestElement" && prop.argument?.type === "Identifier") { | ||
| names.add(prop.argument.name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return names; | ||
| } |
There was a problem hiding this comment.
The AssignmentPattern branch already handles default destructured params like ({ children, ...rest } = {}), but there is no test covering that path. Adding one would prevent regressions.
| return null; | ||
| } | ||
|
|
||
| function findForwardRefRenderFunction(node, forwardRefNames) { |
There was a problem hiding this comment.
findForwardRefRenderFunction supports nested calls like memo(forwardRef(...)) through recursion. A test for this HOC-wrapped case would increase confidence that future changes don't break the recursion behavior.
| return context.getReports(); | ||
| } | ||
|
|
||
| describe("no-flow-node-custom-render", () => { |
There was a problem hiding this comment.
The test suite doesn't exercise aliased forwardRef imports (e.g. import { forwardRef as fr } from 'react'). Adding a test would verify that the forwardRefNames tracking in ImportDeclaration works as intended.
| @@ -0,0 +1,376 @@ | |||
| import { defineRule } from "oxlint"; | |||
There was a problem hiding this comment.
This file is duplicated in lint/no-flow-node-custom-render.js. I understand this is required because packages/kumo-docs-astro/.oxlintrc.json loads the root plugin while packages/kumo/.oxlintrc.json loads the package plugin, so the duplication is consistent with the existing architecture. No change needed.
|
I've submitted a batch review on PR #618. Summary of feedback:
|
2 participants
Summary
kumo/no-flow-node-custom-renderto enforceFlow.Node render={<CustomComponent />}passthrough requirements.Tests
pnpm --filter @cloudflare/kumo exec vitest run --project=unit tests/lint/no-flow-node-custom-render.test.tspnpm exec tsc --noEmit --target ES2022 --module ES2022 --moduleResolution bundler --jsx react-jsx --strict --skipLibCheck --types vitest --allowJs --checkJs false tests/lint/no-flow-node-custom-render.test.ts lint/no-flow-node-custom-render.jspnpm exec oxlint --config /var/folders/bj/9s0glzp52jvf5230xw5lk1rc0000gn/T/opencode/oxlint-touched.json lint/no-flow-node-custom-render.js lint/kumo-plugin.js packages/kumo/lint/no-flow-node-custom-render.js packages/kumo/lint/kumo-plugin.js packages/kumo/tests/lint/no-flow-node-custom-render.test.tspnpm exec prettier --check .oxlintrc.json .changeset/flow-node-render-lint.md lint/kumo-plugin.js lint/no-flow-node-custom-render.js packages/kumo/.oxlintrc.json packages/kumo/lint/kumo-plugin.js packages/kumo/lint/no-flow-node-custom-render.js packages/kumo/tests/lint/no-flow-node-custom-render.test.ts packages/kumo-docs-astro/.oxlintrc.jsonReviews
bonk has reviewed the change
automated review not possible because: bonk has not reviewed this change yet
Tests
Tests included/updated
Automated tests not possible - manual testing has been completed as follows: n/a
Additional testing not necessary because: n/a