jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.97k stars 2.77k forks source link

[react/no-unknown-property] rule conflicts with `@react-three/fiber` on `<mesh />` and other tags #3423

Closed JustFly1984 closed 2 years ago

JustFly1984 commented 2 years ago

I'm using @react-three/fiber in my app, and updating eslint-plugin-react broke the linting.

You did not took into account custom renderers while introducing this changes.

on <mesh /> false negatives:

Unknown property 'castShadow' found Unknown property 'receiveShadow' found Unknown property 'dispose' found Unknown property 'geometry' found Unknown property 'userData' found Unknown property 'rotation' found

on <meshStandardMaterial /> false negatives:

Unknown property 'metalness' found Unknown property 'roughness' found Unknown property 'side' found

on <planeBufferGeometry />

Unknown property 'args' found

on <meshPhongMaterial />

Unknown property 'flatShading' found Unknown property 'shininess' found Unknown property 'side' found Unknown property 'specular' found

and a lot more.

Please fix support for @react-three/fiber

Meanwhile I had to disable react/no-unknown-property rule

ljharb commented 2 years ago

Since <mesh> is not a standard tag, and isn't a custom element, it shouldn't be used at all by that library.

That said, since React ignores nonstandard tags, so should we, which makes this a duplicate of #3420.

JustFly1984 commented 2 years ago

I also agree that it is a non standard tag, but three.js has massive user base, so could it be a separate option to support react-three-fiber tags and attributes? They do have good typescript support.

ljharb commented 2 years ago

I don't think it's worth the complexity - there's multiple libraries that do things they shouldn't by introducing invalid tags. Three.js may want to build an eslint plugin just to check their own tags, though - then they can own their own decisions.

vincent-lecrubier-skydio commented 2 years ago

there's multiple libraries that do things they shouldn't

Who says they shouldn't? Many Custom react renderers exist and I am not aware of anyone saying this shouldn't be done. If anything, the react team has been supportive.

ljharb commented 2 years ago

The HTML standard says they shouldn't, and it's the authority on that.

iamtofr commented 2 years ago

i think u should define these props in a rule to allow/ignore them like:

"react/no-unknown-property": [1, { "ignore": ["position"] }]

brianzinn commented 1 year ago

Hi, Is this going to be accepted as an issue? I'm trying to work out how to support in a renderer for another 3d engine. https://github.com/brianzinn/react-babylonjs/issues/249

I've added to the JSX Intrinsic Elements from my library and VS code has intellisense and works very well with these custom properties. There are a lot of renderers that are NOT rendering HTML, so they will also start to fail...

ljharb commented 1 year ago

@brianzinn there's nothing to accept - the rule already has an ignore config, which you can use to ignore any nonstandard properties you like.

If you want free reign to name properties anything you like, then components need to be named in PascalCase, since lowercased components in react are only HTML elements.

brianzinn commented 1 year ago

ok - thanks for responding - appreciate that. I'll probably need to ignore 1,000 properties. Unfortunately it looks like you don't understand how renderers work. PascalCase are components handled by react. lowercase components are intrinsic elements handled by the renderer (not just the DOM renderer). In other words, lowercased components in react are NOT only HTML elements. Your profile says you are obsessed with backwards compatibility, so bit surprising, but thanks again for clarifying! cheers.

ljharb commented 1 year ago

I understand how renderers work; we only support the react renderer in eslint-plugin-react.

coobeeyon commented 1 year ago

@ljharb would you mind ELI5 what is the difference between a <mesh> and any other React component which I author? I seem to have no problem making custom props for my own stuff. Why wouldn't <mesh> be able to do so too?

CodyJasonBennett commented 1 year ago

I understand how renderers work; we only support the react renderer in eslint-plugin-react.

This would more specifically be strictly react-dom, and this is not future proof to custom elements support AFAIK (this should be a separate issue). For those working in an x-platform codebase, TypeScript (or a plugin that understands TypeScript) would be of more help here.

ljharb commented 1 year ago

@coobeeyon custom compoments are defined by their author; html elements are defined by the browser.

CodyJasonBennett commented 1 year ago

Not cool. This is needlessly contrarian and frankly unprofessional. If you only want to support a subset of JSX that's fine, but don't play this blame game with anything outside of this model.

ljharb commented 1 year ago

@CodyJasonBennett I’m not sure what you mean. Anything lowercased either conforms to the html standard or is invalid, incorrect, and bad - that’s just the way react works.

CodyJasonBennett commented 1 year ago

That is not true. I suggest learning how React and host components work; React is x-platform in nature.

ljharb commented 1 year ago

I’m aware that react 19 may support custom html elements, but I’m not aware of react supporting that yet.

Either way, web components in jsx simply wouldn't be statically analyzable, so if you’re using those, you’re probably not going to get much benefit from eslint.

coobeeyon commented 1 year ago

Well the components in question are certainly designed by the author not the consortium. Shouldn't that mean they can define their own properties? Is the issue really that they are lower case?

ljharb commented 1 year ago

The grammar of jsx says that lowercased elements are solely the province of the renderer, and it is simply not possible for authors to define them, so yes, that is the entire issue. Components created by developers must ALWAYS start with an uppercase letter; components starting with a lowercase letter belong entirely to the renderer.

coobeeyon commented 1 year ago

Well it seems like there are a lot of libraries which are not currently compliant. From what I can see it'd be a very small chance to add a "lenient" or "legacy" setting which would skip checking lower cased components if they weren't defined in the standard. And that change would help the lives of a lot of folks.

kaloyanBozhkov commented 1 year ago

+1 on this, I am on the same boat

rivertam commented 1 year ago

React isn't a renderer but a reconciliation algorithm. react-three-fiber isn't doing something terribly niche with React; it's just not react-dom. React Native and projects like react-blessed have been using this renderer-agnostic aspect of JSX/React for far longer than React 19; I'm surprised this specifically didn't come up in November. It's certainly not true that anything lowercase conforms to the html standard; I don't even think that's true for react-dom.

I don't really know how this relates to eslint in particular, but yeah this basically makes this plugin eslint-plugin-react-dom, not eslint-plugin-react.

ljharb commented 1 year ago

In react, anything lowercase can’t be a custom component, which is why React Native uses non-lowercase components for its primitives like Text etc.

React is react-dom (RN specifically doesn’t clash with react-dom, on purpose). It’s not eslint-plugin-jsx, it’s eslint-plugin-react, and that means the react ecosystem. This isn’t a renderer-agnostic plugin, nor are most things in the ecosystem.

You are welcome to disable the rule if it doesn’t work for you.

JustFly1984 commented 1 year ago

The thing is that the rule works great in all other parts of an app, except custom renderers. It is annoying to add eslint ignore comments for every non html intrinsic element. And it was working ok before some time ago.

ljharb commented 1 year ago

I can certainly understand the annoyance - the "working ok for custom renderers", unfortunately, was "not working properly for the standard renderer".

Do you have a better suggestion beyond listing elements in ignore that wouldn't be a footgun for the primary use case?

JustFly1984 commented 1 year ago

is it possible to ignore non-standard lowercase primitives? is it possible to extend the list of primitives, providing valid attributes list for each of primitive? preferably specifying attribute value types

ljharb commented 1 year ago

The former would be a potential footgun - the latter, though, seems workable, albeit complex.

JustFly1984 commented 1 year ago

I guess each non-standard renderer eventually will provide their own config to extend the rule, if there is an API to satisfy the need.

ljharb commented 1 year ago

Unfortunately eslint rule configuration doesn’t compose, so you’d have to manually merge together various configs :-/

JustFly1984 commented 1 year ago

This is advanced stuff, so I guess advanced people can handle to manually setup the config by copy-pasting somebody's solution. We already configuring an advanced list of plugins and rules for eslint.

There should be a way to use custom renderers. Three.js and react-three-fiber are perfect combination for performant 3d in canvas.

I've attempted 3d in react multiple times through 8 years, but it was always not ready for production, until react-three-fiber has been created. It is popular enough, and constantly maintained to be at least considered essential for developers.

drcmda commented 1 year ago

these assumptions

I’m aware that react 19 may support custom html elements React is react-dom mesh is not a standard tag Anything lowercased either conforms to the html standard or is invalid

are 100% wrong @ljharb, and it would be so easy for you to verify. lowercase in react has nothing to do with html or custom html elements. it designates a native host element. each renderer defines these for its host platform. this is fundamentally how react works.

renderers in react are a first class principle, react-dom adheres to the exact same rules every other renderer, including fiber. they all create a reconciler form defining their lowercase host elements. react-dom defines div and span, fiber mesh and group, and there are countless of others.

ljharb commented 1 year ago

@drcmda yes, I’m aware of that, and the only renderer this rule supports is react-dom. Otherwise, how could we have “unknown” or known properties on an unknown renderer?

cyrilchapon commented 1 year ago

Just landed here, coming from here, with the exact same use-case.

Just a developing-user point of view :

is it possible to extend the list of primitives, providing valid attributes list for each of primitive? preferably specifying attribute value types

This seems like the natural way to go, from a user point of view. Which would unlock react-three-fiber / [any other renderer team] to provide such a list to go in there.

React is react-dom (RN specifically doesn’t clash with react-dom, on purpose)

Well no, as illustrated by this conversation.

So I guess this rule worked "by chance" for many years; and now another package — react-three-fiber — does happen to clash with react-dom. On purpose too, I guess; since a <span> wouldn't have much sense in a 3D WebGL context. It's written as a renderer should be; the react/no-unknown-property rule is not written as it should be.

Therefore, the community needs here is a fix, not a random opinion about renderer casing.

The former would be a potential footgun - the latter, though, seems workable, albeit complex.

I can give it a shot, though I never contributed here. What do you mean "complex" ? Any chance you can point the right direction to start ?

Thanks

ljharb commented 1 year ago

The react/no-unknown property makes the perfectly reasonable and virtually universally true assumption that react-dom is being used for non-custom JSX components.

The complexity is in designing a schema that can cover "any random hallucination some renderer can come up with" while still being trivial to configure to common use cases, and while avoiding confusing, slow, and hard to maintain code in the rule itself.

cyrilchapon commented 1 year ago

Baby steps are the key here 🙂 9 months after the initial issue, you're beginning to consider the root of the trouble might after-all be just an "assumption" and not that "truth from the standards god" you argued in this thread about 10 times.

That's a great start !

Your nonsense-adverbs skills in sentences are quite impressive; but in the real world, that assumption — even if indeed "reasonable" and "true" (please notice how it's much less impressive without the nonsense adverbs) — is completely off-topic.


Let's try another approach, maybe to speed up the intellectual process just a little bit — after all, we can admit actual progress after those 9 months :

=> ✨ @react-three/fiber is not rendering to react-dom !!! ✨


The complexity is in designing a schema that can cover "any random hallucination some renderer can come up with" while still being trivial to configure to common use cases, and while avoiding confusing, slow, and hard to maintain code in the rule itself.

Thanks for this quite constructive "starting point" I asked for 🙂 For sure, a monkey couldn't have told the same answer. Great progress here too !

In the real world though :

{
  "tagsMapping": {
    "@react-three/fiber": ["mesh", "..."]
  }
}
ljharb commented 1 year ago

@cyrilchapon I've never argued anything like "truth from the standards god", and frankly your patronizing tone is hostile and unwelcome.

Your tone makes me want to state that this rule will NEVER, EVER accommodate this use case. Is that the outcome you're hoping for?

cyrilchapon commented 1 year ago

@ljharb at least this gets some attention.

That patronizing tone might just be an answer to the childish tone... ?

Come on man, I'm basically repeating an argument that has been stated here like 20 times.

Be assured that was nothing more than trying to make it funny though, as a desperate call.

ljharb commented 1 year ago

Not the kind of attention you want, I assure you.

cyrilchapon commented 1 year ago

Well grow up, or at least rename that joke eslint-react-dom-plugin.

FalconPilot commented 1 year ago

I can predict the appearance of a new package, probably called jsx-eslint-fiber, just because this one doesn't support one rule... :/

I can understand the reasoning behind the complexity of implementation, but when the reasoning doesn't correspond to the real-world application, isn't it a problem on the long run?

JustFly1984 commented 1 year ago

Guys, I'm a topic starter, I'm also a maintainer of open source project, I do it more than 5 years. I've burned out not once, and there is not much people who has time or skills to contribute. There is also not much donations, or other ways to monetize.

This is an open source, you can fork this package, change the rule, and publish it under different name, and post the link here. But be mindful that you will have to maintain it in long run.

I'm not using react-three-fiber in my current projects so I have no incentive. But for those who use it, this issue is just a huge bummer, especially for juniors. 3d is hard enough, even without tooling issues.

ljharb commented 1 year ago

I agree - and it's unfortunate for those users that fiber would choose to use lowercase tag names, instead of capitalizing what are custom component names.

brianzinn commented 1 year ago

I don't see the casing as unfortunate. That's how host elements are differentiated between React Components. It's unfortunate that unknown elements aren't just ignored automatically - from my experience it's confusing that this plugin complains about some properties, so it's not a good DX. Likely most react renderers include their own JSX Intrinsic Elements, so it's not even strictly necessary for eslint as editors will pick up with intellisense/autocomplete?

ljharb commented 1 year ago

Hmm, that actually might be a reasonable compromise. Is there any overlap between fiber's made-up element tag names and HTML element tag names? If not, we could add an option to ignoreUnknownHostElements.

brianzinn commented 1 year ago

Thank-you @ljharb for being open to more dialog. In the case of my react renderer - I renamed any known ones to avoid overlap. So, I don't use known element names like button, image, line, etc - quite a few conflicted ie: polygon with React.SVGProps<SVGPolygonElement>.

I think the option is a really good idea, but would prefer an opt-out mechanism as opposed to opt-in (ie: default enabled). With the advent of Web Components and other custom elements feel that the linting is best used for known HTML elements such as div, span, etc. I can't speak for other renderers, but if they overlap with known elements then that's for them to fix I think...

JustFly1984 commented 1 year ago

Maybe please reopen an issue?

ljharb commented 1 year ago

@brianzinn to avoid a breaking change it would have to be opt-in. I generally agree that if there's an overlap with html it's a bug in the custom renderer, but it's still useful to know if there's any existing overlaps to worry about.

@JustFly1984 It's not yet clear that's the right path, but if we decide on a direction then i'll do so.

FalconPilot commented 1 year ago

I think that, reading the issue's history, one of the problems has been to mix up HTML W3C standards with JSX/React/React-dom standards.

In theory, that's a sound strategy, as in convention, core HTML nodes are lowercased, and custom JSX components are CamelCased. However, we must keep in mind that this is a convention, not an actually strict rule if you use regular JS. However, things go even further when we start using TypeScript...

People using TypeScript will be familiar with the following:

Capture d’écran 2023-05-02 à 22 26 29

In that example, I created a simple blah FunctionComponent, and rendered it into a Foo component. What happens, then? The above error, in TypeScript.

const blah: React.FC = () => {
  return (
    <div>Blah</div>
  )
}

export const Foo: React.FC = () => {
  return (
    <blah/>
  )
}

My hot take on it would be that JSX actually considers any lowercased component to only be contained within the JSX core library and that any library willing to implement lowercased-components have to override this definition. I'm merely speculating, but my point is, this is another case where conflicting views on how the ecosystem should work is the root problem here.

In a world where the tooling is most of the time adding more complexity due to the inherent nature of the projects it's bound to, I think that just either removing/opting-in problematic rules or simply letting the users override them themselves are the two easiest, but also most realistic solutions.

... Or we could all start programming our web apps in Elm, PureScript or ReasonML and not have to deal with this anymore. Hey, I have a right to dream, right? ¯_(ツ)_/¯

brianzinn commented 1 year ago

@ljharb - Is it not a breaking change that unknown elements are currently errors - isn't that what created this issue in the first place? I don't recall having this issue until recently, so it seems like opt-out would be backwards compatible with the original behaviour - please correct me if I'm wrong there!! 😄

@FalconPilot My understanding is that the JSX.IntrinsicElements are typically extended. ie:

declare global {
  namespace JSX {
    interface IntrinsicElements {
      blah: YourTypeHere
   }
  }
}
FalconPilot commented 1 year ago

@brianzinn Correct, however, it needs to be explicitly extended, which adds another layer of complexity 🤔 Because it means that the library implementing custom, lowercased components has to extend it and the Linter has to acknowledge those new components, which is, IMHO, way too hazardous.

The "CamelCase" convention, while handy, showed its limits as soon as not everyone seemed to adopt it, alas - that's the problem of a convention being only implicitly shared among a developers community. But hey, that problem is pretty much JS in a nutshell (and just literally got imported into JSX), so I'm not pointing fingers at anyone specifically