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

[Bug]: react/prop-types not working when prefixing function with underscore _ #3560

Closed
2 tasks done
KubiGR opened this issue Apr 6, 2023 · 9 comments
Closed
2 tasks done
Labels

Comments

@KubiGR
Copy link

KubiGR commented Apr 6, 2023

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

When naming function components with an underscore _ prefix, the react/prop-types rule does not report anything.
Other prefixes, like $ don't seem to create this problem.

function _EventsList({ eventDate }) {
    return (
        <div>
            {eventDate.events.map((event) => (
                <Event key={event.id} eventId={event.id} />
            ))}
        </div>
    );
}

Expected Behavior

I would expect the rule to appear regardless of the function name.

eslint-plugin-react version

v7.31.10

eslint version

v7.32.0

node version

v10.15.3

@KubiGR KubiGR added the bug label Apr 6, 2023
@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

Are you sure components can start with an underscore? Is <_EventsList /> even valid?

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

(To be clear, the function name absolutely matters; anything that can't be used in jsx isn't a component)

@KubiGR
Copy link
Author

KubiGR commented Apr 6, 2023

Components can start with an underscore. It's true that it goes against standards because components should be in PascalCase.

Let me explain my reasoning and see if it makes sense. I am using a state library (mobx) that needs to wrap with HOCs all your components if you want to use it. I wanted to use the same name, so I could export as the wrapped but I also wanted to use the react/prop-types rule so I needed to create a separated named function. It's true that a prefix in PascalCase could work but even mobx in their docs use this example.

const _MyComponent = props => <div>hi</div>
export const MyComponent = observer(_MyComponent)

I fully understand that the component detection makes sense only in PascalCase but why is $ a prefix that works and _ a one that does not?

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

Components shouldn't be arrow functions anyways - do this:

function MyComponent(props) { return <div>hi</div>; }
const observed = observer(MyComponent);
export { observed as MyComponent }

That said, if <_MyComponent /> works in React, then it's a bug that we don't support it. Can you confirm?

@KubiGR
Copy link
Author

KubiGR commented Apr 6, 2023

I don't know if that's what you are asking me to show but check it out https://codesandbox.io/s/relaxed-taussig-fivkl2?file=/src/App.js.

Yeah, arrow functions should not be used. I have seen you mention this a lot and I agree fully.
I like the export renaming. I will try that for sure.
Do you have any idea regarding the support with $ prefix and not the _ one? Because this was the intriguing thing for me.

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

Since your codesandbox clearly shows that it works, it's thus an oversight/bug in this plugin :-)

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

aha, an explicit check was added in 6b42731/#3335 due to #3334.

I think instead of ignoring things that start with _, we need to look for the next letter and see if it's capitalized.

@ljharb ljharb closed this as completed in 1fc7d34 Apr 6, 2023
@zisiszikos
Copy link

Components shouldn't be arrow functions anyways - do this:

function MyComponent(props) { return <div>hi</div>; }
const observed = observer(MyComponent);
export { observed as MyComponent }

That said, if <_MyComponent /> works in React, then it's a bug that we don't support it. Can you confirm?

I don't think that generally this is a good idea, because what if I wanted to export my original component function too? For example for using it at a testing file. I will get an error that MyComponent has already been exported.

@ljharb
Copy link
Member

ljharb commented Apr 7, 2023

Obviously you can’t export two things under the same name, so my suggestion allows you to trivially rename either or both.

That said, exporting things only for testing is a very bad practice - if it’s never used in production without the wrapper, it should never be tested without the wrapper.

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

No branches or pull requests

3 participants