refactor(nodeOps): remove predicates/optional chaining, use is/context

andretchen0 commented 3 weeks ago

Problem

nodeOps has a few cases where it does type checking that could be pushed down into the called function, making nodeOps simpler and the function more robust.

const deregisterCameraIfRequired = (object: TresObject) => {
  if ((object as unknown as Camera).isCamera) {
    context.deregisterCamera(object as unknown as Camera)
  }
}

Solution

Push predicate into the function:

context.deregisterCamera(child)

Problem

nodeOps contains quite a bit of optional chaining, making code more difficult to read/write than necessary.

Solution

Factor out the need for some optional chaining, e.g., with guards.

if (!node) { return }

Problem

Tres doesn't have a way to test/assert some commonly used types: TresObject, Object3D, Fog, etc. In some cases, it relies on THREE's is* fields, but this leads to more complicated/error-prone code than necessary. For example, this ...

if (child?.isObject3D) {

... can throw an error if the dev forgets the optional chaining (?) and child is undefined.

Solution

Add src/utils/is.ts and accompanying tests.

This does not require optional chaining and will not throw if child is undefined.

if (is.object3D(child)) {

Problem

nodeOps currently does not have a Scene or TresContext by default. It must wait until a passed node arrives, containing those objects.

Solution

Pass TresContext (which contains Scene) to nodeOps.

netlify[bot] commented 3 weeks ago

Deploy Preview for tresjs-docs ready!

Name	Link
Latest commit	7a4a733a5439a74348ca9e530b16557459e7de0e
Latest deploy log	https://app.netlify.com/sites/tresjs-docs/deploys/6663088c93dcba0008f0c278
Deploy Preview	https://deploy-preview-726--tresjs-docs.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

andretchen0 commented 3 weeks ago

Hey @alvarosabu , @garrlker

Thanks for the reviews! I think all the raised issues have been covered with the latest push.

Tresjs / tres