-
-
Notifications
You must be signed in to change notification settings - Fork 53
eslint bug #1136
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
eslint bug #1136
Conversation
|
""" WalkthroughThe ESLint configuration was updated to introduce TypeScript-specific overrides, including parser, parserOptions, plugins, extends, and rules for Changes
Sequence Diagram(s)sequenceDiagram
participant ESLint
participant TypeScriptFile
participant TypeScriptParser
participant TypeScriptPlugin
ESLint->>TypeScriptFile: Detects .ts/.tsx file
ESLint->>TypeScriptParser: Uses @typescript-eslint/parser
ESLint->>TypeScriptPlugin: Applies @typescript-eslint rules
TypeScriptPlugin-->>ESLint: Enforces TypeScript-specific linting
ESLint-->>TypeScriptFile: Reports results with custom rule handling
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.eslintrc.cjs(4 hunks)
🔇 Additional comments (3)
.eslintrc.cjs (3)
47-47: Good extension of story files configuration for TypeScript support.The changes appropriately:
- Extend the file patterns to include TypeScript story files (
.stories.ts,.stories.tsx)- Disable TypeScript-specific rules that might be too restrictive for story files
- Maintain consistency with the existing JavaScript story file configuration
This ensures TypeScript story files have the same linting flexibility as JavaScript ones.
Also applies to: 50-51
59-59: Proper extension of test files configuration for TypeScript.The changes correctly:
- Include TypeScript test file patterns (
.test.ts,.test.tsx)- Disable the TypeScript-specific
no-unused-varsrule for test files where unused variables are common- Maintain parity with the existing JavaScript test file configuration
This provides appropriate linting flexibility for TypeScript test files.
Also applies to: 63-63
119-119: Minor formatting improvements applied.The formatting changes improve code consistency:
- The
semirule is now in a more concise single-line format- Removed an unnecessary comment from the
jsx-a11y/no-autofocusrule- Minor formatting cleanup
These changes don't affect functionality but improve readability.
Also applies to: 139-139, 152-152
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
105-107: ESLint & TypeScript ESLint dependency updates
You’ve added@typescript-eslint/eslint-plugin/@typescript-eslint/parserand bumpedeslintto match your.eslintrc.cjsoverrides. Ensure your ESLint config points to the new parser/plugin versions.Consider using more constrained version ranges (e.g.,
"~8.33.1") or documenting the minimum required versions for reproducible CI builds.Also applies to: 110-110
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(5 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🔇 Additional comments (2)
package.json (2)
3-3: Version bump is correct
The package version has been updated from0.0.43to0.0.44, aligning with the new release.
41-41: ReorderedThemeexport for consistency
MovingThemeto followTextAreais a harmless, stylistic adjustment and maintains alphabetical or logical grouping.
|
merge conflicts |
I will look into it. |
@kotAPI resolved. Please, look into it and give me feedback, thank you. |
|
@kotAPI |
.eslintrc.cjs
Outdated
| files: ['**/*.test.js', '**/*.test.jsx', '**/*.test.ts', '**/*.test.tsx'], | ||
| rules: { | ||
| 'no-unused-vars': 'off', | ||
| '@typescript-eslint/no-unused-vars': 'off', // Add this for TS test files |
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.
maybe let this rule be?
.eslintrc.cjs
Outdated
| files: ['**/*.stories.js', '**/*.stories.jsx', '**/*.stories.ts', '**/*.stories.tsx'], | ||
| rules: { | ||
| 'no-unused-vars': 'off', | ||
| '@typescript-eslint/no-unused-vars': 'off', // Add this for TS story files |
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.
lets not update any of the rules?
we want to keep these on
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.
@kotAPI I have fixed it.
Hello, the issue that we got on storybook regarding the Meta still stands right? From the PR, you've just updated a couple of rules and turned off rules - we're looking for a more permanent solution/better code quality. We don't want the problem to disappear. I hope I'm clear. |
I actually updated some rules to avoid giving that error, but you just asked me to turn it off "no unused var in story". Does my code have an issue? Isn't the fix enough? |
What about the original Meta issue? |
Okay, I am understanding you now. I will figure a way around it and get back to you on it after it's fixed. |
Merge branch 'eslint-bug' of github.com:Dagdon/ui into eslint-bug
|
@Dagdon sorry, I dont think you understood the issue - we are looking for code to be updated - not ignore issues on eslint config One Q - Can you find a solution to use storybook files without making changes to eslint config? Only core contributors to the library are allowed to make eslint changes |
This is a much better explanation, all my thoughts was that you didn't want me to turn off the unused vars rules. So, am I udating just the code of the file with meta issues or together with other files? If there are other files, what files? |
As I mentioned, we are going off-track here. Maintainers reserve the access to update eslint config - for someone's first contribution - an eslint config change is probably not in good faith. You haven't understood the exact issue to solve and I feel this is not good use of everyone's time. Will be closing the PR |
Fixed the eslint bug.
Summary by CodeRabbit