feat(core): add onMount instance callback

[CodyJasonBennett]

Adds an onMount callback for one-of effects for synchronization (i.e. Camera#lookAt, Geometry#translate, etc.). This is called once safely mounted or whenever reconstructed (i.e. args change).

<element onMount={(self) => ...} onUpdate={(self) => ...} />

TODO:

• add lifecycle tests (include suspense if able)
• add or amend docs section (needs to be linkable)

[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 29865cf:

Sandbox	Source
example	Configuration

[hmans]

I'm being very nitpicky, please forgive me:

• as I understand the PR description, onMount will get called on initial mount, but also every time the component is rerendered (props change etc.). If this is the case, I think there's some potential for confusion between the names onMount and onUpdate (the latter of which I imagine could be expected to be invoked after re-renders, which is not what it does -- right?)
• I'm a little worried about potential conflicts with future versions of Three itself (in case it ever decides to add a prop of the same name.)

Is using onMount equivalent to adding a useEffect(callback) to the surrounding component?

[CodyJasonBennett]

onUpdate is called on props change in applyProps, onMount whenever the element is finalized and ready to add to the scene (will be recalled whenever the instance is recreated -- e.g. args change). This is useful for effects that you'd only want to be called once after creation:

<camera onMount={(self) => void self.lookAt(1, 2, 3)} />
<geometry onMount={(self) => void self.translate(1, 2, 3)} />

The onMount equivalent would be:

const elementRef = React.useRef()
const previous = React.useRef()

React.useLayoutEffect(() => {
  // Bail if already ran for this instance
  const element = elementRef.current
  if (previous.current === element.uuid) return

  // Run sync effect
  console.log(element.uuid)

  // Mark instance as updated
  previous.current = element.uuid
})

<element ref={elementRef} />

I know Krispy asked for something like onCreated for elements that prompted this PR, would a name like that be clearer to the behavior described here? It then could be misleading as something like a constructor effect rather than a synchronization effect, but I do think it's important for people to understand that it is a one-of-callback and disassociate it from something like useEffect.

[drcmda]

im not sure if i like it, even onUpdate was too much and i wish i didn't add it, especially as it started to conflict with a real world method called "onUpdate" that some three objects have. adding lifecycles to threejs elements other than interaction events seems a little out of order. imo this would be a better place and less confusing.

useLayoutEffect(() => void ref.current.callMethod(), [])

there's a temptation to bridge or lessen the impact of the imperative nature of some three elements and i think r3f shouldn't encourage it as declarative and imperative can co-exist harmoniously through hooks. i would also fear that once we start adding lifecycles it'll be a can of worms that will never stop: onUnMount, onBeforeMount, onAfterMount, onBeforeUnmount, ...

all in all, i would prefer to stick to how react categorises after-render effects.

[CodyJasonBennett]

im not sure if i like it, even onUpdate was too much and i wish i didn't add it, especially as it started to conflict with a real world method called "onUpdate" that some three objects have

I worry that this is a wider issue of R3F with how it architects instances. This wouldn't happen if the instance wasn't the actual three object. I wouldn't consider rewriting R3F for the sake of either prop, but this is definitely something for a future major nonetheless.

adding lifecycles to threejs elements other than interaction events seems a little out of order. imo this would be a better place and less confusing.

useLayoutEffect(() => void ref.current.callMethod(), [])

This confuses me about the existence of onUpdate. In hindsight, was this by mistake? Is this used enough to foresee continued support? The gotchas behind onMount are pretty specific to R3F/three since elements have constructor args and can be exchanged beneath their respective react components. That raises design issues of their own if misused.

Also, speaking of gotchas and to clarify, that effect can go out of sync with the underlying instance if it's ever exchanged. It can also be disastrous if called more than once on an instance with destructive methods. One might think to just create an instance in a useMemo, but that is very prone to memory leaks if disconnected from the scene graph.

there's a temptation to bridge or lessen the impact of the imperative nature of some three elements and i think r3f shouldn't encourage it as declarative and imperative can co-exist harmoniously through hooks. i would also fear that once we start adding lifecycles it'll be a can of worms that will never stop: onUnMount, onBeforeMount, onAfterMount, onBeforeUnmount, ...

all in all, i would prefer to stick to how react categorises after-render effects.

This is an issue unsolved by react and something that we have to solve ourselves. For instance, a react hook version of this would look like:

function useSyncEffect(elementRef, effect) {
  const [previous] = React.useState(() => new Set())

  React.useLayoutEffect(() => {
    const element = elementRef.current
    if (previous.has(element)) return

    effect(element)
    previous.add(element)
  })
}

//

const cameraRef = React.useRef()
useSyncEffect(cameraRef, camera => camera.lookAt(1, 2, 3))

<camera ref={cameraRef} />

I'm not completely satisfied by this since three's design would often require set up outside of the constructor and would need to be re-run whenever the underlying instance changes (which IMO should be an implementation detail). This is not something that React considers in its lifecycle (and we fight against it in reconstruction).

Consequently, a hook like this would be needed in user-land for this case. I'd prefer to have something native to R3F rather than Drei (or the already too-large user-land hooks/utils folder) as this is quite a sore area between react and three and works around R3F specifics.

[drcmda]

i think i don't follow, i don't understand why the hook has to be that complicated. why not

useLayoutEffect(() => {
  // do everything imperative here
}, [])

no set, no checking, just , []) and it will run on mount before paint. as for edge cases and double execution in dev strict mode, has anyone ever faced a problem with this to be noteworthy?

so this is about switching args which changes the ref? which should re-run the effect? but doesn't that make too many assumptions? shouldn't it be a dependency/condition that tells something to, for instance, re-run a lookat?

and then there's also this mrdoob/three.js#20575 (which three-stdlib has since fixed), that was the only ever problem i am aware of in all these years.

This confuses me about the existence of onUpdate

i regard it as a mistake. it just invites people to sidestep the one place that should manage effects: use[Layout]Effect. i added it in the very beginning because i didn't think much about the consequences. in retrospect a parallel, optional lifecycle-api just causes confusion: hooks, props, and three's own (obj.onBeforeRender et al).

[drcmda]

ps here's the original react issue facebook/react#20090 but imo this is something that react and three need to solve. still being used in drei: https://github.com/pmndrs/drei/blob/master/src/helpers/useEffectfulState.tsx for a single component, "Stats" https://github.com/pmndrs/drei/blob/master/src/core/Stats.tsx#L13 because it is dirty.

sorry for all the text and links, just trying to figure out if this is a user land thing that nobody faces or deep down internals more akin to technical debt.

[CodyJasonBennett]

no set, no checking, just , []) and it will run on mount before paint. as for edge cases and double execution in dev strict mode, has anyone ever faced a problem with this to be noteworthy?

so this is about switching args which changes the ref? which should re-run the effect? but doesn't that make too many assumptions? shouldn't it be a dependency/condition that tells something to, for instance, re-run a lookat?

Yes, this is particularly problematic with destructive methods of geometry classes. I generally avoid doing this in three or in anything beyond a useMemo and binding attributes declaratively as vertex transform/compute methods aren't safe to repeat.

I'm not sure if the hook can be simplified to the same effect, it has to brace against interruption/double-execution and then have args as dependencies or store a reference as a render-effect to keep things in sync. You can remove checks depending on which edge cases matter, or hijack the ref callback, but it would be some combination of those checks.

i regard it as a mistake. it just invites people to sidestep the one place that should manage effects: use[Layout]Effect. i added it in the very beginning because i didn't think much about the consequences. in retrospect a parallel, optional lifecycle-api just causes confusion: hooks, props, and three's own (obj.onBeforeRender et al).

The premise behind this PR was whether effects like these should be allowed in the element body since they're aware of their own lifecycles outside of react. I'm not comfortable adding on to onUpdate if it's proved to be problematic, perhaps these should be hooks instead.

[drcmda]

is there a real world use case, a codesandbox, something that exemplifies the issue that react useEffect would create?

[krispya]

For me, the usecase is usually to do with preparing geometry. For my grass I have a more complicated one in an old CS that I useMemo to do safely:

const geometry = useMemo(() => {
    const _geometry = new THREE.PlaneBufferGeometry(bW, bH, 2, joints);
    // Make the grass geom start from the floor.
    const _trans = new THREE.Matrix4().makeTranslation(0, -bH / 2, 0);
    _geometry.applyMatrix4(_trans);
    // Make the grass geom oriented up.
    const _rot = new THREE.Matrix4().makeRotationX(-Math.PI / 2);
    _geometry.applyMatrix4(_rot);

    // Fold the grass to give it more dimension.
    const vertices = _geometry.attributes.position.array;
    for (let i = 0; i < vertices.length; i += 3) {
      if (vertices[i + 0] === 0) {
        vertices[i + 1] = bW / 2;
      }
    }

    return _geometry;
  }, [bW, bH, joints]);

And then:

<mesh geometry={geometry}> ... </mesh>

I tried doing this in a useLayoutEffect at first but ran into really bad double execution or HMR issues where the transformations would keep applying over and over. There was the option to undo all the transformations in the return but that is obviously a lot of work for no good reason.

I could extend the PlaneBufferGeometry class and then extend it to R3F and this might be the best solution.

The thought was having an event to run effects based on the R3F lifecycle would alleviate trying to navigate the pitfalls of React and also make for a friendly API. As I understand it, React themselves do not suggest use[Layout]Effect for initialization cases, but synchronization cases. Which is why they suggest the run-once-flag if that is required, but in our case we need an instance-has-reconstructed flag.

[drcmda]

@krispya but this is exactly what use memo is for, why doing this as an effect when you want to memorize a computationally expense op that yields a result?

As for use layout effect, it's being used to access the view after render but before paint, on the dom it's typically for measuring and then applying the result. It's totally fine too run effects here imo. Though in your case, I'd also use memo.

The HMR woes you observe can also have other causes, three doesn't know defaults, once you set an object and remove the prop we can only guess. This has been a long standing issue and we do have a solution but it's spotty.

[krispya]

That makes sense to me. I get why you don't want to mix R3F events with the three JSX. What I take from this conversation is:

1. onUpdate should be deprecated if we decide against onMount.
2. We should describe in the docs some best practices for working within the R3F events and lifecycle. Maybe even provide hooks like Cody suggests ie useSyncOnMount(ref, callback) or useSyncOnChange(ref, callback).

[drcmda]

We should describe in the docs some best practices for working within the R3F events and lifecycle. Maybe even provide hooks like Cody suggests ie useSyncOnMount(ref, callback) or useSyncOnChange(ref, callback).

i would prefer a conservative approach when it comes to extending the api surface since this stuff can add to complexity or perceived complexity quick. on mount in react is useEffect(() => ..., []) and im thinking, if there are issues with this approach react-dom probably faces them too. if the issues are prevalent enough react should fix it, or there's a pre-existing thing in use-hooks or sth like that. but re-inventing a mount just in r3f, i would like to see some more real world use cases.

my experience with such helpers has been troubled. r3f had hooks like useUpdate and useResource

react-three-fiber/src/hooks.ts

Lines 36 to 59 in 8eea1b5

	export function useUpdate<T>(
	callback: (props: T) => void,
	dependents: any[],
	optionalRef?: React.MutableRefObject<T>
	): React.MutableRefObject<T> | React.MutableRefObject<undefined> {
	const { invalidate } = useContext(stateContext)
	const localRef = useRef()
	const ref = optionalRef ? optionalRef : localRef
	useLayoutEffect(() => {
	if (ref.current) {
	callback(ref.current)
	invalidate()
	}
	}, dependents) // eslint-disable-line react-hooks/exhaustive-deps
	return ref
	}
	export function useResource<T>(optionalRef?: React.MutableRefObject<T>): React.MutableRefObject<T> {
	const [_, forceUpdate] = useState(false)
	const localRef = useRef<T>((undefined as unknown) as T)
	const ref = optionalRef ? optionalRef : localRef
	useLayoutEffect(() => void forceUpdate((i) => !i), [])
	return ref
	}

but it caused more confusion in the end.

[CodyJasonBennett]

I think the behavior of args should be documented as a gotcha/thing to be accounted for when creating sync effects nonetheless. It's unclear to me whether sync effects are okay to have in effects since react 18 strict mode, so I'm not sure how to go about it.

I wanted to hide this complexity from users, but I see the problems that this can create. Informing them of how to deal with these cases in a react-like way would be an important first step. I'm inclined to close this PR and follow up with an example in the docs of a case where a sync effect might go out of sync if args isn't a dependency of useEffect. I'm not sure of the usefulness of hooks abstractions at this point for informed users.