react/sort-prop-types doesn't work for TypeScript

[amannn]

Since it is mentioned in the README of the rule I expected this to work with TypeScript.

However it seems like it doesn't:

import PropTypes from 'prop-types';
import {Component} from 'react';

const propTypes = {
  b: PropTypes.number,
  a: PropTypes.number // An error is shown here
};

type Props = {
  b?: number;
  a?: number; // No error shown here
};

export class A extends Component<Props> {
  public static propTypes = propTypes;

  public render() {
    const {a, b} = this.props;
    return (
      <p>
        {a} {b}
      </p>
    );
  }
}

export function B({a, b}: Props) {
  return (
    <p>
      {a} {b}
    </p>
  );
}
B.propTypes = propTypes;

Here's a reproduction repository: https://github.com/amannn/eslint-plugin-react-test

Thanks!

[ljharb]

The "sort prop types" rule sorts .propTypes objects; that works fine with TS. The rule isn't meant to sort "the TS type that happens to be assigned to a component's props".

[amannn]

Hmm, in the docs of the rule I found this:

The following patterns are considered warnings:

(...)

type Props = {
  z: number,
  a: any,
  b: string
}
class Component extends React.Component<Props> {
  ...
}

What's the point of this example if the rule doesn't consider it then?

[amannn]

Seems like the type annotations where added here: #2546.

[ljharb]

hmm - @silvenon i guess i didn't check #2546 thoroughly.

There's no tests for TS or Flow types in https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/sort-prop-types.js, so I think this is just a docs error.

I'd be happy to review a PR that either 1) added, behind an option, support for sorting TS/Flow types; or 2) fixes the docs to remove the implication that it sorts TS/Flow types.

[kylemh]

Feels like this typescript-eslint rule handles what you'd want. Would this package still wanna support rules covered by that plugin?

[ljharb]

@kylemh if there's anything react-specific, then this plugin should indeed be able to overlap with that one.

[kylemh]

Sorry, to be clear I totally see the need for the defaultProps and propTypes property sorting. I meant maybe consider not adding the ability to sort type and interface keys from this rule.

[ljharb]

That's a fair point - if that functionality is already doable with typescript-eslint, and there's nothing react-specific that's needed, then this rule probably shouldn't support it.

[amannn]

It seems like member-ordering would enforce it on every type though, right?

In JavaScript, if you want to sort properties of every object, you can use sort-keys. But if you want to do it only for prop types, you can use react/sort-prop-types.

Isn't that a valid use case for TypeScript as well?

[ljharb]

Since prop-types are more like a TS interface than a JS object literal, I'd say it'd make sense to sort TS prop types the same as other TS types?

[kylemh]

My thoughts as well. You can even limit the sorting to only ever sort interface keys too.

My mindset is: if you're sorting the interfaces that represent propTypes in a TS environment, you'd probably also wanna do the same for any interface.

[amannn]

Hmm, I see your point.

My personal opinion is that something like sort-keys or member-ordering is a bit too aggressive and might decrease readability for some cases. For React components, I like to add some consistency with react/jsx-sort-props and react/sort-prop-types and feel like the combination of these two increase the DX. But maybe that's just me 🤷‍♂️

I guess something you could do, is using member-ordering only in files with the .tsx extension. However I'm currently using the .tsx extension even for non-React files (like Discord) – but that might be a downside of that approach.

[kylemh]

Well, that wouldn't work either since you could have non-prop related interfaces in .tsx files, but yeah this is an interesting one!

[amannn]

Yep, there might be some false positives!

[kostia1st]

I like to add some consistency with react/jsx-sort-props and react/sort-prop-types and feel like the combination of these two increase the DX. But maybe that's just me 🤷‍♂️

@amannn
Not just you. I second your point - consistency is important in code readability, and would be greatly appreciated.

And react/sort-prop-types just seem to plain not work with TS prop types, for now.

[amannn]

I think this was addressed in #3615 (requires checkTypes: true)