RFC: @react-three/eslint-plugin rules

[CodyJasonBennett]

Aligning with our docs for general performance pitfalls and API usage around context (use R3F hooks within Canvas):

• no-clone-in-frame-loop: Prefer creating temporary objects in global space and copy rather than clone in hot paths. This should be restricted to three.js classes to avoid collisions. feat(eslint-plugin): add no-new-in-loop and no-clone-in-loop rules #2710
• no-fast-state: Don't set state within loops or continuous events (startTransition can be used if you must, this can be disabled for specific polling cases)
• prefer-useloader: Prefer useLoader for suspense and caching rather than calling Loader.load or Loader.loadAsync in an effect. This will de-dup resources on both the CPU and GPU and avoid later expensive runtime compilation.

[itsdouges]

Thanks for putting this up @CodyJasonBennett.

• For no-fast-state can you give an example of valid/invalid code, as well as usage with startTransition? (edit: ah https://docs.pmnd.rs/react-three-fiber/advanced/pitfalls goes into it, all good)
• Can you give an example of no-hooks-outside-of-canvas? That sounds like a hard rule to lint against, can it be statically known if there is a violation?
• What is the args prop for use-array-args?

[krispya]

Correct me if any of this is wrong Cody.

* Can you give an example of `no-hooks-outside-of-canvas`? That sounds like a hard rule to lint against, can it be statically known if there is a violation?

For example it is not uncommon for someone to try calling useThree in the same component that Canvas is being returned. All hooks should be inside of the Canvas because that's where the provider for the R3F store is.

function App() {
  const ref = useRef()
  // This will fail
  const state = useThree()

  // This will fail
  useFrame(() => {
    ref.current.rotation.y += .01
  }, [])

  return (
  <Canvas>
    <primitive ref={ref} />
  </Canvas>
)}

* What is the `args` prop for `use-array-args`?

He means the args props for three intrinsics. They are always an array in the order the params are ingested into the three class constructor.

<mesh>
   <boxGeometry args={[0.01, 0.01, 0.01]} />
</mesh>

[itsdouges]

Oh of course, the args prop 😅. Thanks Krispy for the examples. I don't think we can effectively lint the hooks outside of canvas flow, it would be better for them to just throw if they were called... do they not do that today?

[krispya]

Yup, they throw errors.

[itsdouges]

no-clone-in-loop and no-new-in-loop have been merged: #2710

Picking up no-fast-state next.

[CodyJasonBennett]

You should be able to update my initial comment if you want to track progress there.

[itsdouges]

Ah yep! Will do next time.

[itsdouges]

@CodyJasonBennett I'm thinking to remove no-hooks-outside-of-canvas and use-array-args from the RFC as they can be covered by dev runtime checks/TypeScript types. Is there anything there that would benefit from static analysis?

[itsdouges]

Working here for the no-fast-state rule https://github.com/pmndrs/react-three-fiber/pull/2724/files#diff-68afedcc7ac7e2c951dd7e84dde7c1e1a55ae8e12d275d5ca88308a3c4dace87

For the use/start transition point can you give example code that would be OK?

Also thinking about more rules from the pitfalls page:

• prefer-frame-loop - would error when using setInterval with small intervals (think 16ms) - if this sounds too restrictive we can not recommend it
• prefer-visibility - would error when conditionally rendering components... definitely wouldn't be in the recommended config. Would be very problematic in a dual DOM/Three codebase.

When I get back to working on my r3f game I'll keep an eye out for patterns that would be useful to call out and lint.

[CodyJasonBennett]

I wouldn't mention startTransition etc. but show a safe polling case which asserts against both the target condition it's listening to and the state it's setting so it only sets once. This would just be an exception, so unsure of whether or how to document in the linter docs.

[itsdouges]

When we say safe polling case I assume we're talking about setting state with a guard, so even though there is state being set in the frame loop/set interval it doesn't matter because it isn't fast. Such as:

const [isOutside, setIsOutside] = useState(false);

useFrame(() => {
  if (position.x > 200 && !isOutside) {
    setIsOutside(true);
  } else if (position.x <= 200 && isOutside) {
    setIsOutside(false);
  }
}); 

Is that on the money? If so for the first pass of the rule I wouldn't try to be too clever, pretty much if set state is guarded (without really checking what the guard is) we ignore the violation.

[CodyJasonBennett]

Only concern would be with strictness (some may want to ban it completely for code style reasons), maybe this is something for a later rule or to be left in user-land.

[itsdouges]

Could be a config option for sure.