Maintenance: Apply stricter eslint rules for unused vars #33004
Conversation
…ce no-unused-vars rule, and adjust consistent-type-imports style
valentinpalkovic
added
ci:daily
valentinpalkovic
changed the title
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many -t check -c production --parallel=7 |
❌ Failed | 9s | View ↗ |
nx run-many -t build -c production --parallel=3 |
✅ Succeeded | 2s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-11 12:28:17 UTC
📝 WalkthroughWalkthroughRepository-wide cleanup: removed unused imports/params, consolidated imports, simplified catch handlers, adjusted function/type parameter names (often underscore-prefixed), fixed test fixtures to evaluate JSON.stringify, and trimmed some story/play and plugin hook arities. No new features added. Changes
Notes: "..." and "*" indicate additional related files with equivalent changes summarized above. Sequence Diagram(s)(Skipped) — changes are widespread refactors and signature cleanups without a single new control-flow feature warranting a sequence diagram. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
code/core/src/csf/story.test.ts (1)
1-1: Consider a more granular approach for unused variables in type-checking files.The blanket
eslint-disablesuppresses allno-unused-varswarnings for the entire file. For a type-checking test file like this, where unused parameters and variables are intentional (e.g.,(storyFn, context)decorators, example components used only for type inference), this may be acceptable. However, the coding guidelines recommend fixing ESLint errors rather than disabling them broadly.Consider whether unused parameters could instead be:
- Prefixed with underscore (e.g.,
(_context)) to explicitly mark them as intentionally unused- Removed if truly unnecessary
- Suppressed granularly with
eslint-disable-next-linecomments on specific linesPlease verify:
- Is this blanket-disable approach consistent across other type-checking/mock test files in this PR?
- Is this the intended strategy for test files that inherently have unused type-example variables?
- Would underscore-prefixed parameters be a better fit given the PR's focus on stricter linting?
code/frameworks/angular/src/client/angular-beta/__testfixtures__/input.component.ts (1)
21-21: LGTM! Underscore prefix correctly indicates unused type parameter.The rename from
Tto_Tproperly follows TypeScript/ESLint conventions for unused generic type parameters and aligns with the PR's objective to enforce stricter unused variable rules.Since this is a test fixture and the type parameter is not used anywhere in the component, you may consider removing it entirely if it doesn't serve a specific testing purpose:
-export class InputComponent<_T> { +export class InputComponent {However, if the generic is intentionally present for API compatibility testing or future extensibility, the underscore prefix is the appropriate approach.
code/frameworks/angular/template/stories/basics/component-with-pipe/custom.pipe.ts (1)
9-11: Correctly removes unused parameter.The removal of the unused
argsparameter is appropriate and consistent with the change incode/frameworks/angular/template/stories/core/moduleMetadata/angular-src/custom.pipe.ts.Note: This pipe implementation is identical to the one in
code/frameworks/angular/template/stories/core/moduleMetadata/angular-src/custom.pipe.ts. If these story examples don't require isolated implementations, consider consolidating them to reduce duplication. However, keeping separate implementations may be intentional for independent story demonstrations.code/core/src/cli/helpers.test.ts (2)
113-113: LGTM – Unused parameter correctly removed.The removal of the unused
pkgparameter from this mock is correct and aligns with the PR's goal of enforcing stricter unused variable rules.Note: This mock implementation is defined inline within the test case. Per coding guidelines, mock implementations should be placed in
beforeEachblocks rather than inside individual test cases. The same pattern appears at lines 92–96. Consider extracting these tobeforeEachfor consistency. Based on coding guidelines.
28-67: Consider addingspy: trueoption to mocks.The coding guidelines specify using
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests. None of the mocks in this file currently use this option.Example for the
node:fsmock:-vi.mock('node:fs', async (importOriginal) => { +vi.mock('node:fs', { spy: true }, async (importOriginal) => { const actual = await importOriginal<typeof import('node:fs')>(); return { ...actual,Based on coding guidelines.
code/frameworks/angular/src/client/angular-beta/ComputesTemplateFromComponent.test.ts (1)
611-623: Consider updating the describe block name.The describe block is named "with argTypes (from compodoc)" but the tests no longer pass
argTypesto the function. While the tests still validate similar prop handling behavior, the name may be misleading.Consider renaming to something like:
- describe('with argTypes (from compodoc)', () => { + describe('with complex prop types', () => {This is optional and can be deferred if preferred.
code/core/template/__mocks__/lodash-es/add.js (1)
1-3: Consider maintaining parameter signature for mock clarity.While removing unused parameters is valid, mock functions typically benefit from maintaining the same signature as the real function for clarity and to avoid confusion. Consider using underscore-prefixed parameters to indicate they're intentionally unused.
-function add() { +function add(_a, _b) { return 'mocked 3'; }
code/core/src/shared/universal-store/use-universal-store-manager.test.ts
Outdated
Show resolved
Hide resolved
valentinpalkovic
force-pushed
the
valentin/apply-stricter-unused-var-eslint-rules
branch
from
9458443 to
44dc6e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/addons/docs/src/blocks/examples/ControlsParameters.tsx (1)
1-1: Suppressing unused-vars warnings contradicts the PR objective.The PR aims to apply stricter ESLint rules for unused variables, but this change disables the rule instead. Since this is an example file demonstrating Controls functionality, the unused parameters should be prefixed with underscores (e.g.,
_a,_b,_d,_f) to indicate they're intentionally declared for type demonstration purposes while still allowing the stricter lint rules to catch genuine issues.Apply this diff to remove the suppression and prefix the unused parameters:
-/* eslint-disable @typescript-eslint/no-unused-vars */ import React from 'react'; type PropTypes = { a?: string; b: string }; -export const ControlsParameters = ({ a = 'a', b }: PropTypes) => <div>Example story</div>; +export const ControlsParameters = ({ a: _a = 'a', b: _b }: PropTypes) => <div>Example story</div>; type SubcomponentAPropTypes = { e: boolean; c: boolean; d?: boolean }; -export const SubcomponentA = ({ d = false }: SubcomponentAPropTypes) => ( +export const SubcomponentA = ({ d: _d = false }: SubcomponentAPropTypes) => ( <div>Example subcomponent A</div> ); type SubcomponentBPropTypes = { g: number; h: number; f?: number }; -export const SubcomponentB = ({ f = 42 }: SubcomponentBPropTypes) => ( +export const SubcomponentB = ({ f: _f = 42 }: SubcomponentBPropTypes) => ( <div>Example subcomponent B</div> );
🧹 Nitpick comments (5)
code/core/src/common/js-package-manager/Yarn1Proxy.test.ts (1)
99-107: LGTM! Unused parameter correctly removed.The change to a no-argument mock implementation is correct—the mock returns a fixed object and doesn't need any parameters. This aligns with the PR's goal of applying stricter ESLint rules for unused variables.
Optional: Consider moving mock implementations to
beforeEach.Per coding guidelines, mock behaviors should be implemented in
beforeEachblocks rather than inline within test cases. This applies to mocks throughout the file (lines 99, 172, etc.). While this is a pre-existing pattern and not introduced by this PR, consolidating mocks inbeforeEachwould improve consistency and maintainability.Based on learnings and coding guidelines.
code/core/src/core-server/presets/favicon.test.ts (1)
57-67: Consider usingspy: truefor package mocks.Per coding guidelines, package mocks should use
vi.mock()with thespy: trueoption. This would allow the original implementations to be spied on while still allowing test-specific overrides.Apply this pattern:
-vi.mock('node:fs', async (importOriginal) => ({ - ...(await importOriginal<typeof import('node:fs')>()), +vi.mock('node:fs', { spy: true }, async (importOriginal) => ({ existsSync: vi.fn(() => { return false; }), statSync: vi.fn(() => { return { isFile: () => false, }; }), }));Note: This is a pre-existing pattern; the current PR changes are appropriate for their scope. Based on learnings.
code/frameworks/nextjs/src/swc/next-swc-loader-patch.ts (1)
137-137: Consider updating for consistency (optional).This line still passes a
nameparameter totraceChild, but the mock at line 51 no longer accepts parameters. While this works fine at runtime (JavaScript ignores extra arguments, andparentTraceis typed asany), updating this call toparentTrace.traceChild()would improve consistency across the mock's usage.Apply this diff if you'd like to align with the parameterless signature:
- const swcSpan = parentTrace.traceChild('next-swc-transform'); + const swcSpan = parentTrace.traceChild();code/core/src/core-server/utils/save-story/mocks/export-variances.stories.tsx (1)
17-19: Clarify whether unused props are intentional test fixtures.The component signature includes
absoluteandborderedprops that are never provided in any story args. If these props serve a testing purpose (e.g., verifying the system handles components with extra props), consider adding a comment. Otherwise, they should be removed to align with the stricter unused vars rules.Apply this diff if the unused props serve no testing purpose:
-const MyComponent: FC<{ absolute: boolean; bordered: boolean; initial: string }> = (props) => ( +const MyComponent: FC<{ initial: string }> = (props) => ( <pre>{JSON.stringify(props)}</pre> );code/core/src/core-server/utils/save-story/mocks/typescript-constructs.stories.tsx (1)
17-19: Remove unused props or document their testing purpose.The
absoluteandborderedprops are unused in all stories. Since this file focuses on testing TypeScript type constructs (as, satisfies, etc.) rather than prop handling, these unused props appear unnecessary.Apply this diff if the unused props serve no testing purpose:
-const MyComponent: FC<{ absolute: boolean; bordered: boolean; initial: string }> = (props) => ( +const MyComponent: FC<{ initial: string }> = (props) => ( <pre>{JSON.stringify(props)}</pre> );
valentinpalkovic
force-pushed
the
valentin/apply-stricter-unused-var-eslint-rules
branch
from
44dc6e2 to
1fe3404
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/frameworks/angular/template/stories/basics/angular-forms/customControlValueAccessor/custom-cva-component.stories.ts (1)
15-19: Replace console.log with Storybook logger or remove debug decorator.This decorator uses
console.log, which violates the coding guidelines for this repository. According to the guidelines, application code undercode/**should use Storybook loggers instead ofconsole.*methods.If this decorator is intended for debugging purposes and should remain, use the Storybook client logger instead:
+import { logger } from 'storybook/internal/client-logger'; + const meta: Meta<CustomCvaComponent> = { // title: 'Basics / Angular forms / ControlValueAccessor', component: CustomCvaComponent, decorators: [ moduleMetadata({ imports: [FormsModule], }), (storyFn) => { const story = storyFn(); - console.log(story); + logger.log(story); return story; }, ], } as Meta;Alternatively, if this decorator was added for debugging and is no longer needed, consider removing it entirely.
As per coding guidelines
♻️ Duplicate comments (2)
code/addons/docs/src/blocks/examples/ControlsParameters.tsx (1)
1-1: Suppressing unused-vars warnings contradicts the PR objective.The PR aims to apply stricter ESLint rules for unused variables, yet this change adds an eslint-disable directive instead of addressing the unused parameters. According to the AI summary, other files in this PR use underscore-prefixing for unused parameters—this file should follow that same pattern for consistency. Since this is an example file for documentation, it should model best practices rather than suppress linting rules.
Apply this diff to align with the PR's pattern and remove the suppression:
-/* eslint-disable @typescript-eslint/no-unused-vars */ import React from 'react'; type PropTypes = { a?: string; b: string }; -export const ControlsParameters = ({ a = 'a', b }: PropTypes) => <div>Example story</div>; +export const ControlsParameters = ({ a: _a = 'a', b: _b }: PropTypes) => <div>Example story</div>; type SubcomponentAPropTypes = { e: boolean; c: boolean; d?: boolean }; -export const SubcomponentA = ({ d = false }: SubcomponentAPropTypes) => ( +export const SubcomponentA = ({ d: _d = false }: SubcomponentAPropTypes) => ( <div>Example subcomponent A</div> ); type SubcomponentBPropTypes = { g: number; h: number; f?: number }; -export const SubcomponentB = ({ f = 42 }: SubcomponentBPropTypes) => ( +export const SubcomponentB = ({ f: _f = 42 }: SubcomponentBPropTypes) => ( <div>Example subcomponent B</div> );code/core/src/csf/csf-factories.test.ts (1)
1-1: Reconsider file-level disable of no-unused-vars rule.Adding a file-level
eslint-disablecontradicts the PR objective of applying stricter unused variable rules. Instead of disabling the rule for the entire file, address specific unused variables by:
- Prefixing intentionally unused parameters with underscore (e.g.,
_args)- Removing genuinely unused variables
- Keeping type parameters needed for type checking (expected behavior)
Apply this approach throughout the file:
-/* eslint-disable @typescript-eslint/no-unused-vars */ //* @vitest-environment happy-dom */Then audit the file for actual unused variables and prefix any intentionally unused function parameters with
_.
🧹 Nitpick comments (1)
code/core/src/types/modules/addons.ts (1)
157-157: Consider using type alias for empty interface.The underscore prefix on
_StoryFnReturnTypeappropriately documents the unused generic parameter. However, the empty interface body can be replaced with a type alias for better semantics.Apply this diff:
-export interface Addon_ClientStoryApi<_StoryFnReturnType = unknown> {} +export type Addon_ClientStoryApi<_StoryFnReturnType = unknown> = Record<string, never>;Or if this serves as an extension point:
-export interface Addon_ClientStoryApi<_StoryFnReturnType = unknown> {} +// eslint-disable-next-line @typescript-eslint/no-empty-object-type +export interface Addon_ClientStoryApi<_StoryFnReturnType = unknown> {}
| name="bar" | ||
| title="foo" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that forwarding title and name to the DOM anchor is intentional.
The updated snapshot shows that title and name props are now being rendered as HTML attributes on the anchor element. These props appear to be Storybook-specific identifiers (used in the SELECT_STORY event at lines 96-97), not necessarily attributes meant for the DOM:
- The
nameattribute on<a>elements is deprecated in HTML5 (replaced byid) - Forwarding internal Storybook identifiers (
title,name) to the DOM may pollute the HTML with unnecessary attributes
This change appears to be a side effect of the ESLint unused variable fix—if title and name were previously destructured to prevent forwarding, removing that destructuring would cause them to spread onto the DOM element.
If this forwarding is unintentional, consider explicitly destructuring these props in link.tsx to exclude them from the rest spread:
const { title, name, ...restProps } = props;
// Use title and name for story selection only
// Spread only restProps onto the anchor elementAlternatively, prefix them with an underscore in the destructuring if they're intentionally unused in that scope:
const { title: _title, name: _name, ...restProps } = props;🏁 Script executed:
#!/bin/bash
# Locate and inspect the LinkTo component implementation in link.tsx
find . -name "link.tsx" -type f | head -5Length of output: 112
🏁 Script executed:
#!/bin/bash
# Inspect the test file to understand context around lines 47, 61-62, and 96-97
cat code/addons/links/src/react/components/link.test.tsx | head -110Length of output: 2353
🏁 Script executed:
#!/bin/bash
# Search for how props are destructured/spread in the LinkTo component
rg -n "title|name" code/addons/links/src/react/components/link.tsx -A 2 -B 2Length of output: 873
🏁 Script executed:
#!/bin/bash
# Get the full link.tsx component to see the render method
cat code/addons/links/src/react/components/link.tsxLength of output: 2074
Destructure title and name in the render method to prevent them from spreading onto the DOM anchor element.
The analysis confirms the review comment is accurate. In code/addons/links/src/react/components/link.tsx, the render() method destructures { kind, story, children, ...rest } but does NOT explicitly extract title and name. This causes them to be included in the ...rest spread onto the anchor element, polluting the DOM with Storybook-internal identifiers.
The name attribute on <a> elements is deprecated in HTML5 and these properties are meant only for story identification in event emissions—not DOM attributes.
Fix by explicitly destructuring title and name in link.tsx line 79:
const { kind, story, title, name, children, ...rest } = this.props;This prevents them from being included in ...rest and spreading to the anchor, consistent with how kind and story are already handled. The test snapshot at lines 61–62 will need to be updated to remove the name and title attributes from the expected anchor output.
🤖 Prompt for AI Agents
In code/addons/links/src/react/components/link.tsx around line 79 and the test
file code/addons/links/src/react/components/link.test.tsx around lines 61–62,
explicitly destructure title and name from this.props in the render method
(e.g., include title and name alongside kind and story) so they are not included
in ...rest and thus not spread onto the anchor element; then update the test
snapshot/expected output in link.test.tsx to remove the now-absent title and
name attributes from the anchor.
| /* eslint-disable @typescript-eslint/no-unused-vars */ | ||
|
|
There was a problem hiding this comment.
Reconsider file-level disable of no-unused-vars rule.
Adding a file-level eslint-disable contradicts the PR objective of applying stricter unused variable rules. Instead of disabling the rule for the entire file, address specific unused variables by:
- Prefixing intentionally unused parameters with underscore (e.g.,
_context) - Removing genuinely unused variables
- Keeping type parameters needed for type checking (expected behavior)
Apply this approach throughout the file:
-/* eslint-disable @typescript-eslint/no-unused-vars */
-
/* global HTMLElement */Then audit the file for actual unused variables and prefix any intentionally unused function parameters with _.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* eslint-disable @typescript-eslint/no-unused-vars */ | |
| /* global HTMLElement */ |
🤖 Prompt for AI Agents
In code/core/src/csf/story.test.ts around lines 1 to 2, remove the file-level
eslint-disable for @typescript-eslint/no-unused-vars and instead handle unused
identifiers locally: delete any truly unused variables, rename intentionally
unused function parameters by prefixing them with an underscore (e.g.,
_context), and retain type-only generics or type imports required for type
checking; audit the rest of the file and apply the same pattern so only needed
identifiers remain and intentional unused params use the underscore prefix.
| @@ -1,5 +1,3 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Please add some elaboration for future reference.
There was a problem hiding this comment.
Due to the way we build our code & how manager-builder is configured.
It was setup in the days before the jsx-runtime was a thing, and has has not been changed since.
| }; | ||
|
|
||
| addons.register(ADDON_ID, (api) => { | ||
| addons.register(ADDON_ID, (_api) => { |
There was a problem hiding this comment.
I personally do not see how this improves things.
TS already warns about unused vars, and they get compiled away at build-time.
I'd be pretty unhappy about unused vars getting removed automatically, unless they start with a _-character, since I often work on new functions and consts first, then the usage.
There was a problem hiding this comment.
I'd be pretty unhappy about unused vars getting removed automatically, unless they start with a _-character, since I often work on new functions and consts first, then the usage.
You don't have to worry about this. Unused args aren't removed automatically. They are just marked as errors. You still have to remove them manually. That's pretty handy because we are forced to consider whether a particular argument is necessary or whether we want to keep it for the function signature.
You're right that TypeScript already warns about that in your IDE, but you don't have that warning during code reviews. It is not evident to the reviewer at first glance whether a variable/argument is used or not. Using an underscore notation improves the readability.
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Chores
Tests
Docs
Chores