Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v9] refactor!: instance descriptors, dynamically map ThreeElements #2465

Merged
merged 68 commits into from
Sep 18, 2022

Conversation

CodyJasonBennett
Copy link
Member

@CodyJasonBennett CodyJasonBennett commented Aug 27, 2022

Continues #2378, dropping architecture changes. Fixes #2250.

Instance Descriptors

This PR creates instance descriptors to separate instance representations from their underlying objects. With them separated, we can avoid mutating objects with flags like dispose={null} and efficiently swap objects at runtime (e.g. in switchInstance). This allows the internal lifecycle of instances to change without breaking user code -- we still create objects in createInstance but could defer that to handleContainerEffects if needed. Instances are an implementation detail of the reconciler, with elements' refs pointed to instance.object.

export interface Instance<O = any> {
root: UseBoundStore<RootState>
type: string
parent: Instance | null
children: Instance[]
props: InstanceProps<O> & Record<string, unknown>
object: O & { __r3f?: Instance<O> }
eventCount: number
handlers: Partial<EventHandlers>
attach?: AttachType<O>
previousAttach?: any
isHidden: boolean
}

Notably, objects is renamed children and is used for all instances whereas previously it only contained attached children. Furthermore, memoizedProps is renamed props and indiscriminately contains elements' props.

Dynamically mapped ThreeElements

Additionally, three.js types for JSX.IntrinsicElements are no longer hard-coded and dynamically mapped to ThreeElements. This can let us relax the three peer dep significantly and make proper use of semver with types with the most recent feature we use being THREE.ColorManagement of r139 and ambient @types/webxr of r141. Consequently, the ReactThreeFiber type namespace is removed with elements' props being defined within ThreeElements.

import type * as THREE from 'three'
import type { EventHandlers } from './core/events'
import type { InstanceProps, ConstructorRepresentation } from './core/renderer'
type Mutable<P> = { [K in keyof P]: P[K] | Readonly<P[K]> }
type NonFunctionKeys<P> = { [K in keyof P]-?: P[K] extends Function ? never : K }[keyof P]
type Overwrite<P, O> = Omit<P, NonFunctionKeys<O>> & O
interface MathRepresentation {
set(...args: any[]): any
}
interface VectorRepresentation extends MathRepresentation {
setScalar(s: number): any
}
type MathProps<P> = {
[K in keyof P]: P[K] extends infer M
? M extends THREE.Color
? ConstructorParameters<typeof THREE.Color> | THREE.ColorRepresentation
: M extends MathRepresentation
? M extends VectorRepresentation
? M | Parameters<M['set']> | Parameters<M['setScalar']>[0]
: M | Parameters<M['set']>
: {}
: {}
}
interface RaycastableRepresentation {
raycast(raycaster: THREE.Raycaster, intersects: THREE.Intersection[]): void
}
type EventProps<P> = P extends RaycastableRepresentation ? Partial<EventHandlers> : {}
interface ReactProps<P> {
children?: React.ReactNode
ref?: React.Ref<P>
key?: React.Key
}
type ElementProps<T extends ConstructorRepresentation, P = InstanceType<T>> = Partial<
Overwrite<P, ReactProps<P> & MathProps<P> & EventProps<P>>
>
export type ThreeElement<T extends ConstructorRepresentation> = Mutable<
Overwrite<ElementProps<T>, Omit<InstanceProps<InstanceType<T>>, 'object'>>
>
type ThreeExports = typeof THREE
type ThreeElementsImpl = {
[K in keyof ThreeExports as Uncapitalize<K>]: ThreeExports[K] extends ConstructorRepresentation
? ThreeElement<ThreeExports[K]>
: never
}
export interface ThreeElements extends ThreeElementsImpl {
primitive: Omit<ThreeElement<any>, 'args'> & { object: object }
}
declare global {
namespace JSX {
interface IntrinsicElements extends ThreeElements {}
}
}

Unified ThreeElement interface

When using extend, the Object3DNode, BufferGeometryNode, MaterialNode, LightNode, and Node interfaces are unified to just ThreeElement<typeof CustomElement>. ThreeElement does not care about the element type, but will map properties based on their signature which best represents the behavior of applyProps.

import { type ThreeElement, extend } from '@react-three/fiber'

declare module '@react-three/fiber' {
  interface ThreeElements {
    customElement: ThreeElement<typeof CustomElement>
  }
}

extend({ CustomElement })

Stable container-effects

Moves attach and interactivity to handleContainerEffects which will fire when trees are linked and finalized. This ensures that effects are not duplicated due to suspense and complete outside of commit, so we're never late and present an incomplete tree. Detailed in #2483.

Internal Stability

As a follow-up to #2491, I've added coverage for utils and the reconciler lifecycle. This is basically a rewrite of renderer tests to fully cover the basics and ensure they work in difficult situations we've designed around.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 27, 2022

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 ee6bf62:

Sandbox Source
example Configuration
append-bug Issue #2250

@CodyJasonBennett CodyJasonBennett changed the title refactor: use instance descriptors [v9] refactor: use instance descriptors Aug 27, 2022
@CodyJasonBennett CodyJasonBennett changed the title [v9] refactor: use instance descriptors [v9] refactor!: instance descriptors, dynamically map ThreeElements, JSX.IntrinsicElements Aug 28, 2022
@CodyJasonBennett CodyJasonBennett changed the title [v9] refactor!: instance descriptors, dynamically map ThreeElements, JSX.IntrinsicElements [v9] refactor!: instance descriptors, dynamically map ThreeElements Aug 28, 2022
@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Sep 13, 2022

@joshuaellis, I'm not very clear on some of the semantics of RTTR's API. getInstance would return the nearest class component or host component (native element), but not function components. Would you prefer to look the Fiber up by the element passed to create and return its instance if possible (or null)? I'd like to improve type safety and predictability in this area.

@joshuaellis
Copy link
Member

@CodyJasonBennett it's largely modelled of react-test-renderer which explicitly discusses the absence of functional components because they don't have an instance. Do you see a benefit in your approach? I'm not sure how much it's used?

@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Sep 13, 2022

To clarify, I mean to do Fiber traversal based on the literal JSX passed to create. So, a function component would return null for getInstance and we can type it accordingly.

I wanted to clarify the intent behind class components since it was not clear whether getInstance referred to host components (native elements) or any component with instance state (this would also include portals). I would prefer to type native elements as Instance here, in any case, similar to useInstanceHandle.

@joshuaellis
Copy link
Member

I wanted to clarify the intent behind class components since it was not clear whether getInstance referred to host components (native elements)

I agree, I don't think portals would be included. I don't believe they are in rtr.

To clarify, I mean to do Fiber traversal based on the literal JSX passed to create. So, a function component would return null for getInstance and we can type it accordingly.

Sounds good to me!

@CodyJasonBennett
Copy link
Member Author

@joshuaellis, @drcmda, @gsimone, @krispya, any objections or withstanding issues before merging? Changes are mostly internal, but I've documented the implications and added test coverage for all the moving parts. I would break this up into another PR, but I didn't want to wait on coverage.

@CodyJasonBennett CodyJasonBennett merged commit 1e7e0a1 into v9 Sep 18, 2022
@CodyJasonBennett CodyJasonBennett deleted the refactor/instance-descriptors branch September 18, 2022 11:44
bigmistqke added a commit to solidjs-community/solid-three that referenced this pull request Jul 18, 2023
r3f refactored their internal typings of Instance during pmndrs/react-three-fiber#2465
- before: Instance represented a THREE-object with additional parameters: __r3f: LocalState, children, remove, add and raycast
- now: Instance: LocalState with additional parameter: object, props and children
    - object: is the THREE-object with additional parameter __r3f ( = a circular reference back to this Instance)
    - props contains the props of the component, without children and ref
    - children contain all the children of the component (before children only contained the attached children)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants