Skip to content

jsx-sort-props regression breaks callbacksLast option. #1175

@phyllisstein

Description

@phyllisstein

I noticed after upgrading to v7.0.0 of this package that the jsx-sort-props rule is no longer honoring my callbacksLast flag. Given this hunk of my ESLint configuration...

  react/jsx-sort-props:
    - warn
    -
      callbacksLast: true
      ignoreCase: true
      reservedFirst: false
      shorthandFirst: true

...a line like this...

<AppBar showLogo={ showLogo } onLogoClick={ this.handleNavigateHome } onToggleMenu={ this.handleToggleMenu } />

...will yield the following warnings:

warning: Props should be sorted alphabetically (react/jsx-sort-props) at src/app/views/app/index.jsx:118:39:
  116 |           <AppMenu />
  117 |         </Popover>
> 118 |         <AppBar showLogo={ showLogo } onLogoClick={ this.handleNavigateHome } onToggleMenu={ this.handleToggleMenu } />
      |                                       ^
  119 |         <main className={ styles.main }>
  120 |           <div className={ styles.mainWrapper } ref={ node => this.scroller = node }>
  121 |             <Switch>


warning: Props should be sorted alphabetically (react/jsx-sort-props) at src/app/views/app/index.jsx:118:79:
  116 |           <AppMenu />
  117 |         </Popover>
> 118 |         <AppBar showLogo={ showLogo } onLogoClick={ this.handleNavigateHome } onToggleMenu={ this.handleToggleMenu } />
      |                                                                               ^
  119 |         <main className={ styles.main }>
  120 |           <div className={ styles.mainWrapper } ref={ node => this.scroller = node }>
  121 |             <Switch>

My expectation would be that with callbacksLast enabled, the showLogo prop should appear before the on... props, which latter should be sorted alphabetically at the end into onLogoClick then onToggleMenu. Though I could certainly be misunderstanding some change to the rule.

Thanks in advance for your help, and let me know if I can furnish any more debugging information!

Activity

jseminck

jseminck commented on May 23, 2017

@jseminck
Contributor

The problem seems to be the combination of ignoreCase: true and callbacksLast: true

callbacksLast is looking for a pattern: on[A-Z]:

function isCallbackPropName(name) {
  return /^on[A-Z]/.test(name);
}

However, ignoreCase: true is calling toLowerCase():

          if (ignoreCase) {
            previousPropName = previousPropName.toLowerCase();
            currentPropName = currentPropName.toLowerCase();
          }

          ... other code ....
 
           if (callbacksLast) {
              var previousIsCallback = isCallbackPropName(previousPropName);
              var currentIsCallback = isCallbackPropName(currentPropName);
           }

The bug was introduced in 7.0.0. Specifically here - where the check for the isCallbackPropName() was moved from before if (ignoreCase) to after this condition.

6e18e40#diff-bf26edcfae7aa27411d9a791b4eb8a8eL74

Seems like a simple fix, moving it back to it's original position does not break any tests. Will create a PR that includes a test case containing these two properties as defined options.

23 remaining items

added a commit that references this issue on Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @ljharb@phyllisstein@jseminck

        Issue actions

          jsx-sort-props regression breaks callbacksLast option. · Issue #1175 · jsx-eslint/eslint-plugin-react