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

Eslint giving error for duplicate props #10064

Closed
mehrdad-shokri opened this issue Jan 27, 2018 · 16 comments
Closed

Eslint giving error for duplicate props #10064

mehrdad-shokri opened this issue Jan 27, 2018 · 16 comments
Labels
component: text field This is the name of the generic UI component, not the React module! support: question Community support but can be turned into an improvement

Comments

@mehrdad-shokri
Copy link

This is not a bug request, but rather, it's a discussion on the reasons why you named 2 prop types of TextField almost the same with one being upper case.
According to TextField API,
TextField exposes two prop types with almost same names. InputProps and inputProps.
Currently, my Eslint gives me an error for having duplicate prop type.
Is there any reason to name 2 prop types almost the same?
Can't we rename 1 of them to something more clarifying?
This also gives headaches with linting tools.

Expected Behavior

Eslint or any other linting tool doesn't give me any duplicate error.(react/jsx-no-duplicate-props).

Current Behavior

Eslint throwing duplicate prop error.

Plugin is located here.

##Temporal solution
Yes, you can set ignoreCase to false in .eslintrc but isn't it better to name our props in a more definitive way?
Thank you.

Your Environment

Tech Version
Material-UI 1.0.0-beta30
React ^16.0.0
eslint ^4.8.0
@mehrdad-shokri
Copy link
Author

Also, please note that Airbnb eslint style uses jsx-no-duplicate with ignore case of true.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 28, 2018

please note that Airbnb eslint style uses jsx-no-duplicate with ignore case of true.

@mehrdaad This is key to the issue. https://github.com/airbnb/javascript/blob/53b2d7d245ba4abefc0429bfda4a46f099b9ace5/packages/eslint-config-airbnb/rules/react.js#L89

Can't we rename 1 of them to something more clarifying?

Input refers to the Input component when input refers to the input element.

@mehrdad-shokri
Copy link
Author

mehrdad-shokri commented Jan 29, 2018

Input refers to the Input component when input refers to the input element.

I've already read that in docs.

What I'm suggesting is rename one of the prop types for more clarification.

and also not getting an error from Airbnb style eslint rules.

@oliviertassinari oliviertassinari added the waiting for 👍 Waiting for upvotes label Jan 31, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 31, 2018

@mehrdaad I have added the waiting for users upvotes tag. I'm closing the issue as it unsure if changing the property names will make the situation better. So please upvote this issue if you think it would. Suggestions are even better! We will prioritize our effort based on the number of upvotes.
You can suppress this eslint warning for now.

@oliviertassinari oliviertassinari added support: question Community support but can be turned into an improvement and removed waiting for 👍 Waiting for upvotes labels Mar 14, 2018
@oliviertassinari
Copy link
Member

I have found the following solution to the problem: much cleaner:

<TextField
- inputProps={{
-   foo: true
- }}
  InputProps={{
    bar: true,
+   inputProps: {
+     foo: true,
+   },
  }}

@vaibhavhrt
Copy link

vaibhavhrt commented Jan 21, 2019

Add this to your .eslintrc.json

{
    ...
    "rules": {
        ...
        "react/jsx-no-duplicate-props": [1, { "ignoreCase": false }]
        ...
    },
    ...
}

This will warn when there are two props of same name. Use 2 instead of 1 for error.

@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Jan 21, 2019
@nghtstr
Copy link

nghtstr commented Jan 31, 2019

I think an even better solution is to have InputProps do as what is expected, but change inputProps to NativeInputProps. That is far more descriptive as to what it does. Having it as the same name, but differing only by the case of the property is prone to errors and simple typos. Changing the native element props to NativeInputProps keeps with the standard of having the properties staring with an upper case as well as makes it far more descriptive.

@matthewjwhitney
Copy link
Contributor

I think an even better solution is to have InputProps do as what is expected, but change inputProps to NativeInputProps. That is far more descriptive as to what it does. Having it as the same name, but differing only by the case of the property is prone to errors and simple typos. Changing the native element props to NativeInputProps keeps with the standard of having the properties staring with an upper case as well as makes it far more descriptive.

I've had this same problem and @nghtstr has proposed an ACTUAL solution to a real problem as opposed to ignoring the problem literally by having the linter ignore it and ignoring any testing warnings etc. Temporarily the the property could be copied and have two names with a deprecation warning on the old one and then on a future update finally remove it.

@bobslaede
Copy link

Checkbox has inputProps which in the documentation should behave like InputProps on TextField
Screenshot of Checkbox documentation:
image
Screenshot of TextField documentation:
image

I see it this way: InputProps on TextField should be inputProps and InputProps should be inputAttributes as the documentation describes anyways.

@matthewjwhitney
Copy link
Contributor

@oliviertassinari can we deprecate InputProps and make it inputAttributes or anything other than a duplicate prop??

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2019

@matthewjwhitney What about htmlInputProps? (like the htmlFor React API) cc @mui-org/core-contributors

@matthewjwhitney
Copy link
Contributor

doesn't matter to me as long as it's not a duplicate...

@mbrookes
Copy link
Member

I quite like this suggestion: #10064 (comment) (as did more people than 👍 the original issue. So, perhaps we drop the prop?

@eps1lon
Copy link
Member

eps1lon commented Feb 13, 2019

I quite like this suggestion: #10064 (comment) (as did more people than the original issue. So, perhaps we drop the prop?

The code change looks pretty trivial. Although in my experience moving a prop to the inherited component means for most API readers it doesn't exist.

@oliviertassinari
Copy link
Member

I quite like this suggestion: #10064 (comment)

@mbrookes This solution requires more effort to be found without landing on this issue. It's non-obvious. A developer needs to look at the Input properties API. It's why we have added inputProps properties to the TextField component. @eps1lon How would you approach the problem?

@eps1lon
Copy link
Member

eps1lon commented Feb 13, 2019

@eps1lon How would you approach the problem?

It's important to understand that the text field is a simple abstraction on top of the following components:

We can't do much beyond that. If people skip the very first paragraph then recovering from that is hard.

change this in the api docs:

const inputProps = {
  step: 300,
};

-return <TextField id="time" type="time" inputProps={inputProps} />;
+return <TextField id="time" type="time" InputProps={{ inputProps }} />;

Add a link to the Input API for the InputProps entry. We do this for all the other components too and it doesn't seem to be an issue for those component props.

Aside:
We could add a functionality to the props table that would expand the immediate props to all available props via prop forwarding. TypeScript users already get this for free via IntelliSense (although without prop description (yet)). Would be nice to see everything in one window instead of opening 2 or 3 tabs to see every possible prop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! support: question Community support but can be turned into an improvement
Projects
None yet
Development

No branches or pull requests

8 participants