brianzinn / react-babylonjs

React for Babylon 3D engine
https://brianzinn.github.io/react-babylonjs/
812 stars 102 forks source link

Eslint - Unknown property on components (react/no-unknown-property) #249

Closed DefaultV closed 1 month ago

DefaultV commented 1 year ago

Hello again! Using Eslint with react-babylon seems to cause some confusion with properties.

It might be happening due to the naming format used by react-babylonjs which matches the one for native components - lowercase with dashes as separators.

A workaround for now for me was using: "react/no-unknown-property": "off" in the eslint config

Bug repro https://github.com/DefaultV/create-react-app-typescript-babylonjs/tree/bug-eslint

.../create-react-app-typescript-babylonjs/src/BouncySphere.tsx
...
   49:7   error    Unknown property 'diameter' found                     react/no-unknown-property
   50:7   error    Unknown property 'segments' found                     react/no-unknown-property
   51:7   error    Unknown property 'position' found                     react/no-unknown-property
   ... And More
...

Let me know if this is something that you intend to change or decide that it's a "wontfix" since it's not really a react-babylon bug

Wish you a great day!

brianzinn commented 1 year ago

i'm so glad you tracked that down - looks pretty buried in eslint. I think it may just be that eslint needs to see the JSX intrinsic elements that react-babylonjs exposes. I'll look more into it for sure. I wouldn't say it's a "wontfix" as I'd like the library to be easy to integrate and work with standard tools like eslint.

brianzinn commented 1 year ago

if you notice in the BouncySphere.tsx the "button" control is called "babylon-button" due to a conflict with HTML button. I needed to do that with a few elements, so it looks like maybe with React18 that more intrinsic elements are available that conflict?

rohitatcosm commented 1 year ago

Yes it is happening with React18 only.

brianzinn commented 1 year ago

just to confirm - there is no solution in this library. the eslint-plugin-react considers non-HTML as part of a breaking change. i think it's good to leave open for visibility, because surely others will have this issue. feel free to comment/emoji on the linked issue above even though it's closed and author suggests similar workarounds to OP.

ljharb commented 1 year ago

There's nothing to fix here, because indeed, these aren't valid HTML elements.

Custom components in jsx must start with a capital letter; lowercase ones are only for standard HTML.

You can use the ignore config to ignore these properties, but I'd recommend fixing the bug in the nonstandard renderer.

brianzinn commented 1 year ago

@ljharb - lowercase ones are NOT only for standard HTML as I said in your project - I really wish you would take the time to understand renderers! only the non-PascalCase "host elements" are handed off to the renderer by react-reconciler, so they MUST be lowercase/camelCase. ie: we MUST have <arcRotateCamera ... /> (and it doesn't require an import). If we have <ArcRotateCamera ... /> then React will look for that SFC/FC. The "host elements" we are using must be lowercase. ReactDOM will get "span" and do a createElement. This library will get an "arcRotateCamera" and do new ArcRotateCamera(name, {options: ...}, scene). We are building 3D scenes declaratively for a real-time 3D engine using HTML5 canvas and native and also immersive XR experiences all thanks to the magic of react-reconciler. I think you have some misconceptions about these renderers and if you take the time to understand what they are doing then you would understand our plight. At a minimum I hope you would at least stop saying that "lowercase ones are only for standard HTML", because it's incorrect from the point of view of anyone using or working on renderers. There are much bigger libraries than this with the same issue.

ljharb commented 1 year ago

Fair enough - then, for the react renderer, only HTML elements may be lowercased, and eslint-plugin-react is for the react renderer.

brianzinn commented 1 year ago

Sounds good then!