-
Notifications
You must be signed in to change notification settings - Fork 10
Add no-deprecated-flash ESLint rule to warn against Flash component usage #384
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: bd99191 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: pksjce <[email protected]>
docs/rules/no-deprecated-flash.md
Outdated
return ( | ||
<Banner | ||
variant="warning" | ||
sx={{fontSize: 0, borderRadius: 0, py: 2, px: 3}} |
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.
remove this sx prop as we are discouraging those too.
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.
Removed the sx prop from the documentation example as requested. The example now only shows variant and className props. (427e5b2)
errors: [{messageId: 'flashDeprecated'}, {messageId: 'flashDeprecated'}], | ||
}, | ||
], | ||
}) |
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.
Also consider a case where other stuff is already being imported from primer/react/experimental
like TooltipV2
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.
Added a test case for existing imports from @primer/react/experimental like TooltipV2. The rule now properly handles scenarios where other experimental components are already being imported. (427e5b2)
src/rules/no-deprecated-flash.js
Outdated
recommended: true, | ||
url: url(module), | ||
}, | ||
fixable: 'code', |
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 don't think this issue is always fixable so just throw a warning in lint for now and remove all the logic to fix this lint error
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.
Removed all auto-fixing logic as requested. The rule now only throws warnings without attempting to fix the code. Removed fixable: 'code'
from meta and all fix functions from the rule implementation. (427e5b2)
@copilot - Can you fix review comments |
…ntal import test Co-authored-by: pksjce <[email protected]>
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.
Pull Request Overview
This PR adds a new ESLint rule no-deprecated-flash
to warn developers when using the deprecated Flash component from @primer/react
and suggests migrating to the Banner component from @primer/react/experimental
.
Key changes:
- Implements warning-only detection (no auto-fixing) for Flash component usage
- Provides comprehensive test coverage for various import and usage scenarios
- Includes complete documentation with examples
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/rules/no-deprecated-flash.js |
Main rule implementation that detects Flash imports and JSX usage from @primer/react |
src/rules/__tests__/no-deprecated-flash.test.js |
Comprehensive test suite covering valid/invalid scenarios including mixed imports |
src/index.js |
Exports the new rule in the plugin's rule registry |
docs/rules/no-deprecated-flash.md |
Documentation with usage examples and migration guidance |
Comments suppressed due to low confidence (1)
docs/rules/no-deprecated-flash.md:1
- The documentation incorrectly states that this rule provides automatic fixes. According to the PR description, this is a "warning-only" rule without auto-fixing capabilities. The documentation should be updated to reflect that this rule only warns about deprecated usage and does not provide automatic fixes.
# No Deprecated Flash
This PR adds a new ESLint rule
no-deprecated-flash
that warns when the Flash component is used and suggests using the Banner component from@primer/react/experimental
instead.Changes
src/rules/no-deprecated-flash.js
- Detects Flash component imports and JSX usage from@primer/react
Rule Features
The rule detects and warns about:
Key capabilities:
@primer/react
Testing
The implementation includes comprehensive test coverage with scenarios for:
All existing tests continue to pass (293/293) and the code follows project linting standards.
Fixes #383.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.