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

Feature request: sort-prop-types should work on TS Props types #3578

Closed
2 tasks done
tylerlaprade opened this issue May 15, 2023 · 20 comments
Closed
2 tasks done

Feature request: sort-prop-types should work on TS Props types #3578

tylerlaprade opened this issue May 15, 2023 · 20 comments

Comments

@tylerlaprade
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

Description Overview

It currently only works on Class components, which are not in favor in the codebases in which I work. The rule doesn't trigger any warnings on properties on a functional component.

Expected Behavior

It should behave the same on functional components as it does on class components.

eslint-plugin-react version

v7.32.2

eslint version

8.40.0

node version

v16.17.1

@ljharb
Copy link
Member

ljharb commented May 15, 2023

Can you provide example code?

@kshirish
Copy link

Facing the same issue. @ljharb

import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';

const variantMap = {
  filled: {
    primary: 'ui-text-white ui-bg-primary',
    secondary: 'ui-text-white ui-bg-secondary',
  },
  outlined: {
    primary: 'ui-border ui-border-primary ui-text-primary',
    secondary: 'ui-border ui-border-secondary ui-text-secondary',
  },
  text: { primary: 'ui-text-primary', secondary: 'ui-text-secondary' },
};

const sizeMap = {
  sm: 'ui-px-2 ui-py-1 ui-text-xs',
  md: 'ui-px-3 ui-py-2 ui-text-sm',
  lg: 'ui-px-4 ui-py-3 ui-text-base',
};

const Button = ({
  variant = 'filled',
  color = 'primary',
  size = 'md',
  children,
  disabled = false,
  className,
  ...restProps
}) => {
  const variantClass = variantMap[variant][color];
  const sizeClass = sizeMap[size];

  return (
    <button
      className={classNames(
        'ui-rounded',
        variantClass,
        sizeClass,
        {
          'ui-pointer-events-none': disabled,
        },
        className,
      )}
      type="button"
      {...restProps}
    >
      {children}
    </button>
  );
};

Button.propTypes = {
  variant: PropTypes.oneOf(['filled', 'outlined', 'text']),
  color: PropTypes.oneOf(['primary', 'secondary']),
  size: PropTypes.oneOf(['sm', 'md', 'lg']),
  disabled: PropTypes.bool,
  className: PropTypes.string,
  children: PropTypes.oneOfType([
    PropTypes.arrayOf(PropTypes.node),
    PropTypes.node,
  ]).isRequired,
};

Button.displayName = 'Button';

export default Button;

Although, it is highlighting the error Prop types declarations should be sorted alphabetically react/sort-prop-types but --fix option is not fixing it!

@kshirish
Copy link

Here is the .eslintrc.js

module.exports = {
  env: {
    browser: true,
    es2021: true,
  },
  extends: [/*'eslint:recommended'*/ 'plugin:react/recommended'],
  overrides: [],
  parserOptions: {
    ecmaVersion: 'latest',
    sourceType: 'module',
  },
  plugins: ['react'],
  rules: {
    'react/sort-prop-types': [
      2,
      {
        callbacksLast: true,
        sortShapeProp: true,
        noSortAlphabetically: false,
      },
    ],
    'react/jsx-sort-props': [
      2,
      {
        callbacksLast: true,
        shorthandFirst: false,
        shorthandLast: true,
        noSortAlphabetically: false,
        reservedFirst: true,
      },
    ],
  },
};

@ljharb
Copy link
Member

ljharb commented May 30, 2023

@kshirish autofixing isn't ever a guarantee; if it's warning properly then there's no bug, just perhaps a lack of an enhancement.

@ljharb
Copy link
Member

ljharb commented May 31, 2023

ping @tylerlaprade; can you provide example code?

@tylerlaprade
Copy link
Contributor Author

@ljharb, I would have expected this to give me an error since "aaa" should be sorted before "zzz".

type Props = {
  zzz: string;
  aaa: string;
}

function Foo(props: Props) {
...
}

Config:

"react/sort-prop-types": "error",

@ljharb
Copy link
Member

ljharb commented Jun 8, 2023

does it give that error on a class component?

In other words, is this a bug with SFCs, or a bug with TS type definitions? (which i'm not sure the rule ever supported)

@tylerlaprade
Copy link
Contributor Author

I have never used a class component, so I'm not entirely sure of the syntax, but looking at the above comment I made this change and it does fire. However, I don't think many people define their props like that anymore, at least not on any of the projects I have worked on. I really like the rules in this plugin and if possible would enjoy using this one on TypeScript components, since all my projects use them.

image

@ljharb
Copy link
Member

ljharb commented Jun 8, 2023

Foo.propTypes should fire for all kinds of components, including SFCs, but i'm talking about using a Props type specifically.

@tylerlaprade
Copy link
Contributor Author

Yes, I would like to use Props types with this rule - is it possible to add?

@ljharb
Copy link
Member

ljharb commented Jun 8, 2023

That seems like a reasonable enhancement to add.

@ljharb ljharb changed the title [Bug]: sort-prop-types does not work on React Functional Components [Bug]: sort-prop-types does not work on TS Props types Jun 8, 2023
@ljharb ljharb changed the title [Bug]: sort-prop-types does not work on TS Props types Feature request: sort-prop-types should work on TS Props types Jun 8, 2023
@akulsr0
Copy link
Contributor

akulsr0 commented Aug 18, 2023

Should we close this? as this was fixed with #3615

@tylerlaprade
Copy link
Contributor Author

Oh nice! Is it available for me to test out yet? Which version number is it included in?

@ljharb
Copy link
Member

ljharb commented Aug 18, 2023

Yes, merged with #3615. It's not yet released.

@ljharb ljharb closed this as completed Aug 18, 2023
@tylerlaprade
Copy link
Contributor Author

Thanks @ljharb! When will it be released? I see that the documentation has been updated already.

@ljharb
Copy link
Member

ljharb commented Sep 8, 2023

The docs you should be reading are on the version tag you’re using; everywhere on github, the docs on the default branch won’t necessarily be releases.

@tylerlaprade
Copy link
Contributor Author

I see, thanks @ljharb! What is the expected release schedule?

@ljharb
Copy link
Member

ljharb commented Oct 19, 2023

There isn't a schedule; I'll get to it as soon as I can.

@tylerlaprade
Copy link
Contributor Author

Checking up on the status since the fix was pushed over six months ago, but there still have been no new releases. Is there a bottleneck I can help with in the release process? Does it require some sort of back-testing?

@ljharb
Copy link
Member

ljharb commented Feb 27, 2024

Thanks for offering :-) no bottleneck, I just like to get as many PRs into a release as possible. There's 3 open PRs now I'd like to land first.

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

No branches or pull requests

4 participants