[Fix] prop-types: Support typescript props interface extension and TSTypeAliasDeclaration

[hank121314]

Fixes #2654. Fixes #2719. Fixes #2703.

Now support the following typescript syntax for prop validation,
but extension only support with parser @typescript-eslint/parser since typescript-eslint-parser is deprecated.

1. interface extension.

interface GenericProps {
  a: () => void;
}

interface ImplementationProps extends GenericProps {
  b: () => void;
}

export const Implementation: FC<ImplementationProps> = ({
  a,
  b,
}: ImplementationProps) => <div />;

2. interface extension with ReturnType

const mapStateToProps = (state) => ({
  books: state.books,
});

interface BooksTable extends ReturnType<typeof mapStateToProps> {
  username: string;
}

const App = (props: BooksTable) => {
  props.books();
  return (
    <div>
      <span>{props.username}</span>
    </div>
  );
};

ReturnType can only accept one parameter, and the parameter should be function type, and that function should return an ObjectExpression.

3. TSTypeAliasDeclaration and intersectionType

type User = {
  user: string,
};

type Props = User & { userId : string };

export default (props: Props) => {
  const { userId, user } = props;

  if (userId === 0) {
    return <p>userId is 0</p>;
  }

  return null;
};

Thanks you so much for your code review!

[ljharb]

Thanks, this seems great

[ljharb]

Suggested change

	const regexp = new RegExp('^(?:Function(?:Declaration|Expression)|ArrowFunctionExpression)$');
	const regexp = /^(?:Function(?:Declaration|Expression)|ArrowFunctionExpression)$/;

new RegExp should never be used when a literal suffices.

altho this seems like it'd be clearer as a chain of ifs rather than using a regex.

[hank121314]

Thanks for your advice!
How about using Array.includes ?

function isFunctionType(node) {
  if (node.type) {
    return ['FunctionDeclaration', 'FunctionExpression', 'ArrowFunctionExpression'].includes(node.type);
  }
  return false;
}

[ljharb]

That also seems like needless runtime overhead for only three choices. I'd prefer a simple if chain.

[hank121314]

Hi @ljharb !
I refactor some code and use @typescript-eslint/parser instead of typescript-eslint-parser for all test case in prop-types and nounused-prop-types.
Thanks you so much for your code review and Sorry for taking your time! 😭

[ljharb]

@hank121314 in general it's best if we use both - ie, duplicate the test cases - so we can be sure that things stay working as much as possible on the old TS parser.

[ljharb]

specifically, all these test cases with the old TS parser should be left untouched, and duplicated, with the duplicate using the new TS parser.

[hank121314]

OK! I have added it back.
But I think we might need to deprecated old TS parser someday.
The ast tree between two parser have a big difference.
It is really hard to support both parser for their own ast tree.

[ljharb]

Thanks!

[ljharb]

I've rebased the branch for you; the eslint 4 + node 4 tests are failing, but I suspect that's because the new TS parser doesn't work there.

I'll finish updating the branch to handle that tomorrow, and then land it.

[hank121314]

I've rebased the branch for you; the eslint 4 + node 4 tests are failing, but I suspect that's because the new TS parser doesn't work there.

I'll finish updating the branch to handle that tomorrow, and then land it.

Yes, seems that new TS parser only support eslint version greater than 5.
I am so grateful for your help! 😄

[ljharb]

@hank121314 ok, i think i've addressed that problem, but I'm now seeing a few tests failing locally - one example:

          type User = {
            user: string;
          }

          type Props = User & {
          };

          export default (props: Props) => {
            const { userId, user } = props;

            if (userId === 0) {
              return <p>userId is 0</p>;
            }

            return null;
          };

is warning on both userId and user, but should only be warning on userId.

(Please delete your local branch and check it out fresh, so you can work on top of my rebase)

[hank121314]

@hank121314 ok, i think i've addressed that problem, but I'm now seeing a few tests failing locally - one example:

          type User = {
            user: string;
          }

          type Props = User & {
          };

          export default (props: Props) => {
            const { userId, user } = props;

            if (userId === 0) {
              return <p>userId is 0</p>;
            }

            return null;
          };

is warning on both userId and user, but should only be warning on userId.

(Please delete your local branch and check it out fresh, so you can work on top of my rebase)

I'm sorry to say this pr doesn't support old TS parser for any of intersectionType or extension.
Since old TS parser is deprecated so I haven't gave it's support yet 😞 .
If old TS parser support is necessary, I will update this pr.

[ljharb]

@hank121314 ah - so, one option is certainly to comment out the "old TS parser" tests, land this, and fix them later - but i'd prefer to do them all at once, if you're willing :-)

[hank121314]

@hank121314 ah - so, one option is certainly to comment out the "old TS parser" tests, land this, and fix them later - but i'd prefer to do them all at once, if you're willing :-)

It's my pleasure! I have pushed the latest version with old TS parser support and add some test case in no-unused-prop-types.
Thank you for your support and code view once again.

[ljharb]

k, figured out the test failures in eslint 4 :-) rebased again.

[sergeushenecz]

Hi. It not working if extented interface exist in another file.

[ljharb]

@sergeu90 cross-file linting isn't practical.

[sergeushenecz]

@ljharb If I have interface product for example and I want validate props in component product. I need move interface to the file product component right? But if I have multiply places where I want use product then I have global product interface and local product interface in the product component? And if I want to add new property I need add to 2 places?

[ljharb]

Yes, that's just a limitation in linting. It's more clear though to repeat prop names so that someone doesn't have to go read another file to find out what props a component takes.

[sergeushenecz]

@ljharb Thanks for reply