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: handle React.FC, intersection, union types #3241

Merged
merged 1 commit into from
Mar 13, 2022
Merged

[Fix] boolean-prop-naming: handle React.FC, intersection, union types #3241

merged 1 commit into from
Mar 13, 2022

Conversation

mobily
Copy link
Contributor

@mobily mobily commented Mar 11, 2022

Types of changes

This PR fixes #3197, therefore, the boolean-prop-naming rule validates correctly the following code:

import type { FC } from 'react' // or import { FC } from 'react'

type Props = {
  enabled: boolean // ❌ error
} 

export const Component: FC<Props> = props => { 
  return <div /> 
}
import * as React from 'react' 

type Props = {
  enabled: boolean // ❌ error
} 

export const Component: React.FC<Props> = props => { 
  return <div /> 
}
import type { FC } from 'react' // or import { FC } from 'react'

type Props = {
  isEnabled: boolean // ✅
} 

export const Component: FC<Props> = props => { 
  return <div /> 
}
import * as React from 'react' 

type Props = {
  isEnabled: boolean // ✅
} 

export const Component: React.FC<Props> = props => { 
  return <div /> 
}

Moreover, previously, the plugin could not handle a case like this:

import * as React from 'react'
import { BoxProps } from '@mobily/stacks'

type Props = {
  isEnabled: boolean, // ⬅️ this was completely ignored
} & BoxProps

export const Component = (props: Props) => { 
  return <div /> 
}

or

import * as React from 'react'

type Props = {
  isEnabled: boolean, 
} & {
  lol: boolean, // ⬅️ same here, both property types were ignored
}

export const Component = (props: Props) => { 
  return <div /> 
}

Now, the plugin handles the intersection and union types as well, so the following will be validated correctly:

import * as React from 'react'

type Props = {
  isEnabled: boolean, // ✅
} & {
  lol: boolean, // ❌ error
}

export const Component = (props: Props) => { 
  return <div /> 
}
import * as React from 'react'

type Props = {
  isEnabled: boolean, // ✅
} | {
  lol: boolean, // ❌ error
}

export const Component = (props: Props) => { 
  return <div /> 
}
import * as React from 'react'

type Props = {
  isEnabled: boolean, // ✅
} & ({
  lol: boolean, // ❌ error
} | {
  hasLOL: boolean, // ✅
})

export const Component = (props: Props) => { 
  return <div /> 
}

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great!

lib/rules/boolean-prop-naming.js Outdated Show resolved Hide resolved
tests/lib/rules/boolean-prop-naming.js Outdated Show resolved Hide resolved
@mobily mobily requested a review from ljharb March 12, 2022 08:23
@mobily
Copy link
Contributor Author

mobily commented Mar 12, 2022

@ljharb and in the meantime, I have added a logic that handles the union type as well, I'm looking forward to your review!

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems great!

lib/rules/boolean-prop-naming.js Outdated Show resolved Hide resolved
@ljharb ljharb changed the title Fix #3197 and improve the boolean-prop-naming rule [Fix] boolean-prop-naming: handle React.FC, intersection, union types Mar 12, 2022
@ljharb
Copy link
Member

ljharb commented Mar 12, 2022

hmm, looks like the latest changes you pushed up (or, possibly, my tiny tweaks, but i doubt it) are failing some tests. can you take a look?

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2022

Codecov Report

Merging #3241 (b59417f) into master (cdfd558) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3241   +/-   ##
=======================================
  Coverage   97.68%   97.69%           
=======================================
  Files         121      121           
  Lines        8520     8550   +30     
  Branches     3076     3096   +20     
=======================================
+ Hits         8323     8353   +30     
  Misses        197      197           
Impacted Files Coverage Δ
lib/rules/boolean-prop-naming.js 99.40% <100.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdfd558...b59417f. Read the comment docs.

@mobily
Copy link
Contributor Author

mobily commented Mar 12, 2022

✅ and done, thanks, @ljharb for tweaking my code a bit!

edit: not yet 😅

@mobily
Copy link
Contributor Author

mobily commented Mar 13, 2022

@ljharb ok, I think the most important cases have been handled, could you please run all test suites once again?

@ljharb ljharb merged commit 86a3177 into jsx-eslint:master Mar 13, 2022
@mobily mobily deleted the bool-prop-naming-tweaks branch March 13, 2022 08:48
@mobily
Copy link
Contributor Author

mobily commented Mar 13, 2022

thank you @ljharb ❤️

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

Successfully merging this pull request may close these issues.

react/boolean-prop-naming interface/type on React.FC validation of boolean with typescript does not work
3 participants