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

Add option to ignore spread operator in sort-default-props #2178

Closed
VincentLanglet opened this issue Feb 27, 2019 · 6 comments
Closed

Add option to ignore spread operator in sort-default-props #2178

VincentLanglet opened this issue Feb 27, 2019 · 6 comments

Comments

@VincentLanglet
Copy link
Contributor

With foo = { bar: baz }.

{ ...foo, bar } = { bar }

And

{ bar, ...for } = { baz }

So I think, an option to ignore spread operator in sort default props will be great.
Correct example

{ a: 'a', c: 'c', ...foo, b: 'b', d: 'd' }

Incorrect example

{ a: 'a', ...foo, c: 'c', b: 'b', d: 'd' }

Plus, it allow to develop a fixer (asked here) without breaking the code.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2019

I very much don’t agree that this would be good. A correct fixer will never move anything across a spread boundary, and while it’s fine to use linting rules to block language syntax, using linting rules to willfully break language syntax is not ok.

@ljharb ljharb closed this as completed Feb 27, 2019
@VincentLanglet
Copy link
Contributor Author

@ljharb I'm not sure we understand each other.
Why are you talking about willfuly break language syntax ?

I was aking about an option to avoid having error in case like this

bar.defaultProps = { ...foo,  anotherProps }

Cause the rule actually ask for

bar.defaultProps = { anotherProps, ...foo,  }

It's not a good thing if eslint ask for breaking my code and I have to add disable-next-line multiple times in my project.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2019

Gotcha, that makes sense. Thanks for clarifying.

An option to only enforce sorting within spread boundaries seems like a good addition.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Feb 27, 2019

To be clear with everyone

Without the option

{ a, bar, foo } // valid
{ a, foo, bar } // invalid
{ ...foo, a, bar } // invalid
{ a, ...foo, bar } // invalid
{ a, bar, ...foo } // valid
{ bar, a, ...foo } // invalid

With the option

{ a, bar, foo } // valid
{ a, foo, bar } // invalid
{ ...foo, a, bar } // valid
{ a, ...foo, bar } // valid
{ a, bar, ...foo } // valid
{ bar, a, ...foo } // invalid

@VincentLanglet
Copy link
Contributor Author

@ljharb I started to look into the code to implement the option.

In test, we can see

{
    code: [
      'export default class ClassWithSpreadInPropTypes extends BaseClass {',
      '  static propTypes = {',
      '    b: PropTypes.string,',
      '    ...c.propTypes,',
      '    a: PropTypes.string',
      '  }',
      '  static defaultProps = {',
      '    b: "b",',
      '    ...c.defaultProps,',
      '    a: "a"',
      '  }',
      '}'
    ].join('\n'),
    parser: 'babel-eslint'
  }

Is expected to be a valid code. There is a check

if (/SpreadProperty$/.test(curr.type)) {
          return decls[idx + 1];
        }

So I think I found a bug. Cause this test case return an error.

{
    code: [
      'const defaults = {',
      '  b: "b"',
      '};',
      'const First = (props) => <div />;',
      'export const propTypes = {',
      '    a: PropTypes.any,',
      '    z: PropTypes.string,',
      '};',
      'export const defaultProps = {',
      '    ...defaults,',
      '    a: "a",',
      '    z: "z",',
      '};',
      'First.propTypes = propTypes;',
      'First.defaultProps = defaultProps;'
    ].join('\n')
  }

@VincentLanglet
Copy link
Contributor Author

@ljharb I found the fix and made the PR

if (/SpreadProperty$/.test(curr.type) || /SpreadElement$/.test(curr.type)) {
          return decls[idx + 1];
        }

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

No branches or pull requests

2 participants