-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Maintenance: Apply stricter eslint rules for unused vars #33004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| function add(a, b) { | ||
| function add(_a, _b) { | ||
| return 'mocked 3'; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,3 @@ | ||
| import React from 'react'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't do this yet.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add some elaboration for future reference.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Due to the way we build our code & how manager-builder is configured. |
||
|
|
||
| import type { PlayFunctionContext } from 'storybook/internal/csf'; | ||
|
|
||
| import type { Meta, StoryObj } from '@storybook/react-vite'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,8 @@ describe('LinkTo', () => { | |
| expect(container.firstChild).toMatchInlineSnapshot(` | ||
| <a | ||
| href="originpathname?path=/story/foo--bar" | ||
| name="bar" | ||
| title="foo" | ||
|
Comment on lines
+61
to
+62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify that forwarding The updated snapshot shows that
This change appears to be a side effect of the ESLint unused variable fix—if If this forwarding is unintentional, consider explicitly destructuring these props in 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 The analysis confirms the review comment is accurate. In The Fix by explicitly destructuring const { kind, story, title, name, children, ...rest } = this.props;This prevents them from being included in 🤖 Prompt for AI Agents |
||
| > | ||
| link | ||
| </a> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 newfunctions andconsts first, then the usage.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.