fix: type errors with latest three types

[Methuselah96]

Why

Resolves #1381.

This PR avoids expanding the types used as part of React.forwardRef. This is important because the types might differ depending on which version of three.js is being used. If the types are expanded at compile-time (as they are now), they are using the version of @types/three that @react-three/drei depends on as a devDependency, which might cause issues with the consumer's version of @types/three.

I think it would make sense to apply this change to most of the drei components that are using forawrdRef, so I'm opening this as a draft PR before updating the ~70 components that this new approach would be applied to.

What

Introduce ForwardRefComponent type that every component that uses forwardRef can use in order to avoid expanding types at compile-time of the @react-three/drei declaration files.

As an example, before this PR, the Detailed component declaration types look like this:

import * as React from 'react';
import { LOD, Object3D } from 'three';
export declare const Detailed: React.ForwardRefExoticComponent<Pick<Omit<import("@react-three/fiber").ExtendedColors<import("@react-three/fiber").Overwrite<Partial<LOD>, import("@react-three/fiber").NodeProps<LOD, typeof LOD>>>, import("@react-three/fiber").NonFunctionKeys<{
    position?: import("@react-three/fiber").Vector3 | undefined;
    up?: import("@react-three/fiber").Vector3 | undefined;
    scale?: import("@react-three/fiber").Vector3 | undefined;
    rotation?: import("@react-three/fiber").Euler | undefined;
    matrix?: import("@react-three/fiber").Matrix4 | undefined;
    quaternion?: import("@react-three/fiber").Quaternion | undefined;
    layers?: import("@react-three/fiber").Layers | undefined;
    dispose?: (() => void) | null | undefined;
}>> & {
    position?: import("@react-three/fiber").Vector3 | undefined;
    up?: import("@react-three/fiber").Vector3 | undefined;
    scale?: import("@react-three/fiber").Vector3 | undefined;
    rotation?: import("@react-three/fiber").Euler | undefined;
    matrix?: import("@react-three/fiber").Matrix4 | undefined;
    quaternion?: import("@react-three/fiber").Quaternion | undefined;
    layers?: import("@react-three/fiber").Layers | undefined;
    dispose?: (() => void) | null | undefined;
} & import("@react-three/fiber/dist/declarations/src/core/events").EventHandlers & {
    children: React.ReactElement<Object3D>[];
    hysteresis?: number | undefined;
    distances: number[];
}, "visible" | "attach" | "args" | "children" | "key" | "onUpdate" | "position" | "up" | "scale" | "rotation" | "matrix" | "quaternion" | "layers" | "dispose" | "type" | "id" | "uuid" | "name" | "parent" | "modelViewMatrix" | "normalMatrix" | "matrixWorld" | "matrixAutoUpdate" | "matrixWorldAutoUpdate" | "matrixWorldNeedsUpdate" | "castShadow" | "receiveShadow" | "frustumCulled" | "renderOrder" | "animations" | "userData" | "customDepthMaterial" | "customDistanceMaterial" | "isObject3D" | "onBeforeRender" | "onAfterRender" | "applyMatrix4" | "applyQuaternion" | "setRotationFromAxisAngle" | "setRotationFromEuler" | "setRotationFromMatrix" | "setRotationFromQuaternion" | "rotateOnAxis" | "rotateOnWorldAxis" | "rotateX" | "rotateY" | "rotateZ" | "translateOnAxis" | "translateX" | "translateY" | "translateZ" | "localToWorld" | "worldToLocal" | "lookAt" | "add" | "remove" | "removeFromParent" | "clear" | "getObjectById" | "getObjectByName" | "getObjectByProperty" | "getObjectsByProperty" | "getWorldPosition" | "getWorldQuaternion" | "getWorldScale" | "getWorldDirection" | "raycast" | "traverse" | "traverseVisible" | "traverseAncestors" | "updateMatrix" | "updateMatrixWorld" | "updateWorldMatrix" | "toJSON" | "clone" | "copy" | "addEventListener" | "hasEventListener" | "removeEventListener" | "dispatchEvent" | keyof import("@react-three/fiber/dist/declarations/src/core/events").EventHandlers | "objects" | "levels" | "autoUpdate" | "isLOD" | "addLevel" | "getCurrentLevel" | "getObjectForDistance" | "update" | "hysteresis" | "distances"> & React.RefAttributes<unknown>>;

After this PR, the Detailed component declaration types look like this:

import * as React from 'react';
import { LOD, Object3D } from 'three';
import { ForwardRefComponent } from '../helpers/ts-utils';
declare type Props = JSX.IntrinsicElements['lOD'] & {
    children: React.ReactElement<Object3D>[];
    hysteresis?: number;
    distances: number[];
};
export declare const Detailed: ForwardRefComponent<Props, LOD>;
export {};

Checklist

• Documentation updated N/A
• Storybook entry added N/A
• Ready to be merged

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
drei	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 23, 2023 5:05pm

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8ad22e0:

Sandbox	Source
upbeat-shtern-l2vz3q	Configuration
Ground reflections and video textures	Configuration
arc-x-pmndrs-colors	Configuration

[drcmda]

@CodyJasonBennett this looks great, what do you think?

[Methuselah96]

I went ahead and implemented this for all usages of forwardRef. The two considerations worth mentioning are:

1. The order of the P (props) and T (ref type) generics for the ForwardRefComponent utility type. I have them ordered as <P, T> which matches the order of the parameters in forwardRef, but is the opposite of the order of the generics for forwardRef. Let me know if you think I should swap those.
2. This approach introduces some duplication of declaring the generic types twice. This is necessary because the ref type is still included in the props if we only declare the ForwardRefComponent. I can look into whether there's a way to avoid the duplication, but for now just wanted to make you aware of the necessity of declaring the generic types twice.

[MelonCode]

Hey guys, is there anything that prevents this PR from being merged?

[iLambda]

Is there news on the merging of this PR ?
Most of the RenderTexture related features are completely broken (typescript reports a sourceFile property missing), and the react-three documentation recommands using drei for creating portal-related features.

[Methuselah96]

I'm happy to resolve merge conflicts whenever there's a signal from the maintainers that they're open to merging this.

[CodyJasonBennett]

Sorry I missed this. I'm happy to accept these changes, although I worry this is a deeper issue with our configuration and/or TypeScript itself. It's not intended nor intuitive for our dev dependencies to influence consumer code whatsoever, and I foresee this continuing to affect future projects and be a routine issue. Is this a known issue or pitfall that has tracking? I'm admittedly a bit disappointed in the stability of types across our ecosystem and want to stem the bleeding if I can.

[iLambda]

I think it has to do with using the types in @types/three to produce the props for some types (for instance, RenderTexture).

Ideally, it would be good if typescript understood that the types comes not from your dev dependencies, but the actual @types/three the consumer has as a dependency.
Problem is, this behavior can't be implemented, because there is no static enforcement of the fact that the types should be stable depending on the version. Thus, when they get out of sync, this happens in the consumer's codebase.

It doesn't seem like a conceptual problem to me, simply that towers of dependencies can get complicated when typescript is involved. This problem should only occur when @types/three changes the props of intrinsic JSX elements.
I'm sure it can be automatized with CI in most cases, with a if @types/three changed version, try to upgrade. If it doesn't work, ask for a human to look at it

[CodyJasonBennett]

It is absolutely a bug if TypeScript diverges from Node resolution as per consumers' configurations, thus devDependencies should be out of the question. That is precisely my concern and why I find this unavoidably error-prone for the ecosystem, so I'm eager to acknowledge and resolve any issues we're dancing around. Am I perhaps misunderstanding the problem? I only see workarounds for what I can only presume to be a deeper issue, notably we do not want nor benefit from type expansion where JSX is involved as it's dynamic in nature.

[Methuselah96]

TypeScript does not pay attention to whether a package is a dependency or a devDependency, it will emit the same types regardless. I think the TypeScript team would argue that @react-three/fiber and @types/three should be dependencys since the emitted types depend on them.

I also don't think there's a blessed way to control type expansion in the emitted types. microsoft/TypeScript#35654 (which is closed, but I'm not sure it should be), microsoft/TypeScript#47205, microsoft/TypeScript#34778, and microsoft/TypeScript#31940 seem like some related issues to watch on that front.

[iLambda]

Yeah, @Methuselah96 explained it way better than I did. I wasn't aware that it was a well known thing that people are requesting.

[CodyJasonBennett]

It's unfortunate microsoft/TypeScript#35654 doesn't cover this case, but I better understand the motivations for this PR. Thanks.

[Methuselah96]

Merge conflicts resolved.

[github-actions]

🎉 This PR is included in version 9.80.8 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀