Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Add eslint rule to ensure spread props come before other props #437

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

Merged
siddharthkp merged 5 commits into main from copilot/add-eslint-rule-for-props
Oct 24, 2025

Conversation

Copy link
Contributor

Copilot AI commented Oct 24, 2025 •
edited by siddharthkp
Loading

Note

👋 Hello from human co-author. I've reviewed the code and it genuinely looks good.

Implementation Complete: spread-props-first ESLint Rule ✅

This PR successfully implements a new ESLint rule that ensures spread props come before other props in JSX elements to prevent unintentional prop overrides.

Implementation Status

  • Create rule implementation file: src/rules/spread-props-first.js
  • Create test file: src/rules/__tests__/spread-props-first.test.js
  • Add rule export to src/index.js
  • Create documentation: docs/rules/spread-props-first.md
  • Run tests to validate implementation (all 334 tests pass)
  • Run linting to ensure code quality (passes)
  • Manual testing of rule functionality (working correctly)
  • Address code review feedback (removed ESLint version compatibility code)
  • Security scan (no issues found)
  • Add changeset for release tracking

What Was Implemented

Rule Behavior:

  • Detects when spread props ({...rest}, {...props}, etc.) appear after named props
  • Reports an error with clear context about which props need reordering
  • Provides automatic fixing that moves all spreads before all named props

Test Coverage:

  • 7 valid test cases covering correct usage patterns
  • 8 invalid test cases covering various incorrect patterns
  • All tests verify both detection and autofix behavior

Documentation:

  • Clear explanation with examples of correct vs incorrect code
  • Real-world explanation of why prop ordering matters
  • Information about the autofix feature

Files Changed

  • src/rules/spread-props-first.js (81 lines) - Rule implementation
  • src/rules/__tests__/spread-props-first.test.js (123 lines) - Comprehensive tests
  • src/index.js (1 line) - Rule export
  • docs/rules/spread-props-first.md (66 lines) - Documentation
  • .changeset/upset-cobras-cross.md - Changeset for release tracking

Validation Results

✅ All 334 tests pass (including 15 new tests)
✅ ESLint passes
✅ Markdown linting passes
✅ Prettier formatting passes
✅ CodeQL security scan passes (0 alerts)
✅ Manual testing confirms correct behavior

Security Summary

No vulnerabilities detected by CodeQL analysis.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copy link

changeset-bot bot commented Oct 24, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 6e8de23

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-primer-react Minor

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

Copilot AI and others added 2 commits October 24, 2025 12:45 
Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
Copy link
Member

@copilot We only support eslint version > 9, you can remove any code that offers compatibility to older versions

Copilot AI changed the title (削除) [WIP] Add eslint rule to ensure props are spread before other props (削除ここまで) (追記) Add eslint rule to ensure spread props come before other props (追記ここまで) Oct 24, 2025
Copilot AI requested a review from siddharthkp October 24, 2025 12:50
Copilot finished work on behalf of siddharthkp October 24, 2025 12:50 
Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 24, 2025

@copilot We only support eslint version > 9, you can remove any code that offers compatibility to older versions

Done! Removed the ESLint version compatibility code and now using context.sourceCode directly. Fixed in f3fff3b.

Copilot finished work on behalf of siddharthkp October 24, 2025 12:56
siddharthkp

This comment was marked as outdated.

Copy link
Member

@copilot look at other changesets and add a changeset in the same style

 

Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 24, 2025

@copilot look at other changesets and add a changeset in the same style

Added changeset in 6e8de23. It's a minor version bump that describes the new spread-props-first rule.

@siddharthkp siddharthkp marked this pull request as ready for review October 24, 2025 16:42
@siddharthkp siddharthkp requested a review from a team as a code owner October 24, 2025 16:42
@siddharthkp siddharthkp requested review from Copilot, jonrohan and joshblack and removed request for jonrohan October 24, 2025 16:42
Copy link
Contributor

Copilot AI left a 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 spread-props-first that enforces spread props to appear before named props in JSX elements, preventing unintentional prop overrides. The implementation includes comprehensive test coverage and documentation.

Key changes:

  • New ESLint rule with automatic fixing capability
  • 15 test cases (7 valid, 8 invalid) all passing
  • Complete documentation with examples

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/rules/spread-props-first.js Core rule implementation with detection and autofix logic
src/rules/__tests__/spread-props-first.test.js Comprehensive test suite covering valid and invalid cases
src/index.js Export of the new rule
docs/rules/spread-props-first.md User-facing documentation with examples
.changeset/upset-cobras-cross.md Release changeset tracking the new feature

node: spreadAttr,
messageId: 'spreadPropsFirst',
data: {
spreadProp: sourceCode.getText(spreadAttr),
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using sourceCode.getText() to display the spread prop in the error message could be misleading if the spread expression is complex or multiline. Consider extracting just the spread identifier name (e.g., rest from {...rest}) using spreadAttr.argument.name for clearer error messages.

Suggested change
spreadProp: sourceCode.getText(spreadAttr),
spreadProp: spreadAttr.argument&&spreadAttr.argument.type==='Identifier'
? spreadAttr.argument.name
: sourceCode.getText(spreadAttr.argument),

siddharthkp reacted with eyes emoji
Copilot uses AI. Check for mistakes.
Copilot AI requested a review from siddharthkp October 24, 2025 16:43
Copilot finished work on behalf of siddharthkp October 24, 2025 16:43
Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks genuinely good!

Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🥳 Am very interested to try it out.

One question, would it be possible to have the rule (or a follow up rule?) that helps people understand when something may not be merged correctly. For example:

-<Example {...rest} className="..." />
+<Example {...rest} className={clsx(className, '...')} />
-<Example {...rest} onClick={() => {}} />
+<Example {...rest} onClick={compose(onClick, () => {})} />

It would be helpful to know in these situations that we have not merged rest correctly. My guess is that it would be too hard in eslint world to know what rest is but figured I'd ask

Copy link
Member

Ooh! yeah that's a good one. If rest is coming from the same file, it should be possible with reading the types 🤔

Will try it out (or make copilot try it 🤭) in a new issue

@siddharthkp siddharthkp merged commit 9270d40 into main Oct 24, 2025
9 checks passed
@siddharthkp siddharthkp deleted the copilot/add-eslint-rule-for-props branch October 24, 2025 16:49
'enforce-css-module-identifier-casing': require('./rules/enforce-css-module-identifier-casing'),
'enforce-css-module-default-import': require('./rules/enforce-css-module-default-import'),
'use-styled-react-import': require('./rules/use-styled-react-import'),
'spread-props-first': require('./rules/spread-props-first'),
Copy link
Member

@siddharthkp siddharthkp Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: I have intentionally left this out from recommended config, not sure if everyone wants it or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siddharthkp siddharthkp siddharthkp approved these changes

Copilot code review Copilot Copilot left review comments

@joshblack joshblack joshblack approved these changes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add eslint rule to make sure props are spread before other props

AltStyle によって変換されたページ (->オリジナル) /