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

[7.31.4] no-unknown-property throwing on valid props #3384

Closed
diegohaz opened this issue Sep 3, 2022 · 24 comments · Fixed by #3385
Closed

[7.31.4] no-unknown-property throwing on valid props #3384

diegohaz opened this issue Sep 3, 2022 · 24 comments · Fixed by #3385

Comments

@diegohaz
Copy link

diegohaz commented Sep 3, 2022

Related to #3380

I'm seeing the following errors in my project:

Unknown property 'onToggle' found

<details> supports onToggle.

Invalid property 'fill' found on tag 'path', but it is only allowed on: svg

<path> supports fill.

Unknown property 'as' found

<link> supports as.


Edit: It looks like there are even more valid use cases that this rule is not covering. I'd suggest reverting the changes to the no-unknown-property rule for v7 and releasing it under a new major version. React Known Props can be used to get all the known props, and users can use the ignore option on the rule to add other props like css.

@diegohaz
Copy link
Author

diegohaz commented Sep 3, 2022

Might be worth checking this: https://github.com/tcodes0/react-known-props

cc @tcodes0

@dermoumi
Copy link

dermoumi commented Sep 3, 2022

Same thing for onPointerDown (and probably other pointer events https://reactjs.org/blog/2018/05/23/react-v-16-4.html)

@peter-resmed
Copy link

Also having issue with css property of emotion library
image

@Primajin
Copy link
Contributor

Primajin commented Sep 3, 2022

I am also getting this error on defaultChecked

Unknown property defaultChecked found  react/no-unknown-property

In the React rendering lifecycle, the value attribute on form elements will override the value in the DOM. With an uncontrolled component, you often want React to specify the initial value, but leave subsequent updates uncontrolled. To handle this case, you can specify a defaultValue or defaultChecked attribute instead of value.

@Meemaw
Copy link

Meemaw commented Sep 3, 2022

Also this should work, but complains that fill is only allowed on svg:

type Props = {
  fill?: string
}

export const Example = ({
  fill,
}: Props) => (
  <svg fill={fill}>
  </svg>
)

@christian98
Copy link

I'm having the same problem with fill and <rect/>:

 29:29  error  Invalid property 'fill' found on tag 'rect', but it is only allowed on: svg  react/no-unknown-property

See https://developer.mozilla.org/en-US/docs/Web/SVG/Element/rect?retiredLocale=de

@ljharb
Copy link
Member

ljharb commented Sep 3, 2022

@diegohaz Please upgrade to v7.31.4. Are you still seeing these issues?

@Meemaw @christian98 @Primajin @dermoumi a fix for these will be out later today.

@peter-resmed custom attributes that don’t start with data- are invalid - is there an issue filed on emotion for this?

@diegohaz diegohaz changed the title [7.31.3] no-unknown-property throwing on valid props [7.31.4] no-unknown-property throwing on valid props Sep 3, 2022
@diegohaz
Copy link
Author

diegohaz commented Sep 3, 2022

@diegohaz Please upgrade to v7.31.4. Are you still seeing these issues?

Yes

@diegohaz
Copy link
Author

diegohaz commented Sep 3, 2022

custom attributes that don’t start with data- are invalid - is there an issue filed on emotion for this?

React does allow custom attributes without data- in certain circumstances. I guess a lot of people would expect the lint rule to work accordingly to that.

@mjchang
Copy link

mjchang commented Sep 3, 2022

Also getting an issue with the fill attribute on g

@guyellis
Copy link

guyellis commented Sep 3, 2022

I upgraded to 7.31.4 and I'm still seeing this with playsInline with the video element.

Good news is that 7.31.4 fixed all the other false positives.

@chrisweb
Copy link

chrisweb commented Sep 3, 2022

@ljharb @peter-resmed I opened a ticket in the emotion js repository: emotion-js/emotion#2878

also I confirm @guyellis message about playsInline MDN Video HTML Element attribute playsInline, @sjarva could you please add that one too, to your PR #3385? Thank you for all the fixes you already added to that PR

@AviVahl
Copy link

AviVahl commented Sep 3, 2022

7.31.4
This should pass, but doesn't:
<g fill="#7B82A0" fillRule="evenodd">

"error: Invalid property 'fill' found on tag 'g', but it is only allowed on: svg react/no-unknown-property"

@AviVahl
Copy link

AviVahl commented Sep 3, 2022

It also fails when explicitly passing <div children={...} />, which should work just like <div>{...}</div>

@sibartlett
Copy link

I also get Unknown property 'scrolling' found on <iframe>

@ljharb
Copy link
Member

ljharb commented Sep 4, 2022

I think #3385 now covers everything here except:

  • emotion's css property: the only solutions I can think of here are a) emotion users disable this rule, b) hardcode "css" as an allowed property in the rule (ick), or c) allow the rule to be configurable so users can specify their own properties to ignore.
  • react apparently allows everything - which is very unfortunate and bizarre - but if we mimicked that, then there's no point in using this rule at all.

If there's anything I missed, please comment here while the issue remains open - or, file a new issue if this one is closed. v7.31.5 will be released shortly after CI passes.

@lyz810
Copy link

lyz810 commented Sep 4, 2022

Unknown property 'onTouchMove' found
Unknown property 'onTouchEnd' found
Unknown property 'onTouchStart' found

As above, I got three errors on div element, which should be supported by react.

@ljharb
Copy link
Member

ljharb commented Sep 4, 2022

@lyz810 yes, thanks, those are included in #3385.

@Primajin
Copy link
Contributor

Primajin commented Sep 4, 2022

First of all a big thank you for the quick turn around ❤️

Regarding emotion css - I guess the cleanest way would be to feed an ignore pattern or list of things to ignore. css per-se is not a valid HTML attribute but total valid react prop in the context of emotion-css. I'm pretty sure other libraries add other funky attributes / props so I'd vote for allowlisting them. 🤞🏻

@teamkuka
Copy link

teamkuka commented Sep 4, 2022

Hello, I am using the latest version of eslint-plugin-react (^7.31.5), and that one is still to correct
Error: Unknown property 'controls' found react/no-unknown-property (in a video player)

Cheers for the rapid fix :)

@tronikelis
Copy link

Getting an unknown property "controls" error as well, on a video element

@sjarva
Copy link
Contributor

sjarva commented Sep 4, 2022

emotion's css property: the only solutions I can think of here are a) emotion users disable this rule, b) hardcode "css" as an allowed property in the rule (ick), or c) allow the rule to be configurable so users can specify their own properties to ignore.

(Like @diegohaz already said in the modified issue comment) The rule is already configurable with ignore, so the users of emotion should add css as ignored property, when configuring this rule:

...
"react/no-unknown-property": ['error', { ignore: ['css'] }]
...

@ljharb I made a commit that adds this to the rule documentation in #3390

SimeonC added a commit to tablecheck/frontend that referenced this issue Sep 30, 2022
SimeonC added a commit to tablecheck/frontend that referenced this issue Oct 3, 2022
@AMI3GOLtd
Copy link

AMI3GOLtd commented Oct 11, 2022

I'm getting the same error in v7.31.9 on iframe with the allowFullScreen attribute:

Invalid property 'allowFullScreen' found on tag 'iframe', but it is only allowed on: video react/no-unknown-property

@ljharb
Copy link
Member

ljharb commented Oct 11, 2022

@AMI3GOLtd yes, that's fixed by #3455 in v7.31.10. Please file new issues in the future for these things.

ScottG489 added a commit to ScottG489/scottg489.github.io that referenced this issue Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet