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

[Fix] boolean-prop-naming: add check for typescript "boolean" type #2930

Merged
merged 1 commit into from Feb 21, 2021

Conversation

vedadeepta
Copy link
Contributor

Overview of change:

Add a check for typescript boolean type without the need to explicitly define react PropTypes.

type Props = {
  enabled: boolean
}
const Hello = (props: Props) => <div />;

Will throw a linting error for the above code.

@ljharb
Copy link
Member

ljharb commented Feb 21, 2021

(note, the new tests weren't actually running since [].concat wasn't being used; i've fixed that)

@ljharb ljharb changed the title [fix] boolean-prop-naming: add check for typescript "boolean" type [Fix] boolean-prop-naming: add check for typescript "boolean" type Feb 21, 2021
@vedadeepta
Copy link
Contributor Author

(note, the new tests weren't actually running since [].concat wasn't being used; i've fixed that)

that's weird, because it did run locally using this

node node_modules/mocha/bin/_mocha tests/lib/rules/boolean-prop-naming.js

@ljharb ljharb merged commit ae5ace5 into jsx-eslint:master Feb 21, 2021
@ljharb
Copy link
Member

ljharb commented Feb 21, 2021

@vedadeepta the tests completed, but weren't actually running, because parsers.TS() returns an array, and the tests are objects.

@vedadeepta
Copy link
Contributor Author

vedadeepta commented Feb 22, 2021

cropped

This is the output i get when i run the tests without concat on this branch using the command node node_modules/mocha/bin/_mocha tests/lib/rules/boolean-prop-naming.js (I'm using parser.TS() two times separately for the last two invalid tests e.g parser.TS(testObj1), parser.TS(testObj2) instead of parser.TS(testObj1, testObj2)). Should it be something else?

Also i don't think the last test on the master branch for boolean-prop-naming is running at all. Change it to something like this

parsers.TS({
    code: `
    type TestConstType = {
      enabled: boolean
    }
    const HelloNew = (props: TestConstType) => { return <div /> };
    `,
    options: [{
      rule: '^is[A-Z]([A-Za-z0-9]?)+'
    }],
    parser: parsers['@TYPESCRIPT_ESLINT'],
    errors: [{
      message: 'Prop name (enabled) doesn\'t match rule (^is[A-Z]([A-Za-z0-9]?)+)'
    }]
  }, {
    code: `
    type TestFNType = {
      isEnabled: boolean
    }
    const HelloNew = (props: TestFNType) => { return <div /> };
    `,
    options: [{
      rule: '^is[A-Z]([A-Za-z0-9]?)+'
    }],
    parser: parsers['@TYPESCRIPT_ESLINT'],
    errors: [{
      message: 'Prop name (enabled) doesn\'t match rule (^is[A-Z]([A-Za-z0-9]?)+)'
    }]
  })

The last one should fail but it does not. It also shows "61 passing" (+ no failures reported) instead of 62 as in the screenshot above. The last test is completely omitted on master.

I think there is something wrong with parser.TS function. it just returns the first object most probably.

@ljharb
Copy link
Member

ljharb commented Feb 23, 2021

parsers.TS returns either its first argument (which is meant to be an array), or an array.

I'll make some changes so it's harder to misuse.

ljharb added a commit to ljharb/eslint-plugin-react that referenced this pull request Feb 23, 2021
@vedadeepta
Copy link
Contributor Author

Thanks for looking into it.

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

Successfully merging this pull request may close these issues.

Incorrect documentation for boolean-prop-naming
2 participants