Skip to content
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

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

Closed
phyllisstein opened this issue May 7, 2017 · 1 comment
Closed

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

phyllisstein opened this issue May 7, 2017 · 1 comment

Comments

@phyllisstein
Copy link

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!

@jseminck
Copy link
Contributor

jseminck commented May 23, 2017

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.

@ljharb ljharb closed this as completed May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants