fix(R3F version): Resolve compatibility with React StrictMode

[AlaricBaraou]

In react in strict mode for example you can have <TilesPlugin /> useLayoutEffect running twice and registering, unregistering then registering again the plugin causing it to throw the error 'TilesRendererBase: A plugin can only be registered to a single tile set'.

https://react.dev/reference/react/StrictMode#enabling-strict-mode-for-entire-app

As to why this wasn't noticed in earlier version of r3f, the StrictMode wasn't inherited within the r3f component tree prior to v9.
See https://r3f.docs.pmnd.rs/tutorials/v9-migration-guide#strictmode

[gkjohnson]

Thank! I think one issue here is that plugins were never intended to be unregistered and then registered again. Once disposed a new plugin should be created if it needs to be added again (this is one of the reasons this flag is on the class in the first place).

Right now the current life cycle events are intended to guarantee the following operation order:

• Instance is created.
• Members are assigned to the plugin.
• Plugin is registered after being fully initialized.

I'm wondering if we move the "registeration" and "member assignment" step into the same initial "useLayoutEffect" call to avoid this issue. Something like so:

useLayoutEffect( () => {
  const plugin = new PluginConstructor( ...args );
  assignDeepOptions( plugin, options );
  tiles.registerPlugin( plugin );

  return () => tiles.unregisterPlugin( plugin );
}, [ PluginConstructor, tiles, useObjectDep( args ) ] ); // "options" is explicitly ignored so we don't recreate the plugin when the options are changed.

useDeepOptions( instance, options );

What do you think? I'm also wondering if we should switch to use React 19 in the dev dependencies for the sake of testing.

[AlaricBaraou]

I'm also wondering if we should switch to use React 19 in the dev dependencies for the sake of testing.

Probably best to use the latest react and r3f version, but this issue can be tested by adding as a first child of for now. I think some dependencies still ask for @react-three/fiber v8 so I did not change it.

Once disposed a new plugin should be created if it needs to be added again

Then you solution is probably best but it comes with a breaking change for the r3f version of the package I think.
Until now you were guaranteed to have the plugin instance available in the context from the first render while now it will only be available after useLayoutEffect runs.

Same for tiles, to support StrictMode I had to extract it from useMemo and place it in a useEffect.
It's possible to support StrictMode while still using useMemo but the .dispose method was breaking the state of the tiles and I did not figure out why.
But here is an example of Drei of a component that has a similar dispose call and the class inited in a useMemo ( useState but the logic should be the same) https://github.com/pmndrs/drei/blob/4e48be0fe2bba6f81478eba0196d9eb4d2920b62/src/core/Text.tsx#L86

The current commit works with the examples. But maybe there is a better way.

[gkjohnson]

Thanks for the changes!

Until now you were guaranteed to have the plugin instance available in the context from the first render while now it will only be available after useLayoutEffect runs.

Same for tiles, to support StrictMode I had to extract it from useMemo and place it in a useEffect.
It's possible to support StrictMode while still using useMemo but the .dispose method was breaking the state of the tiles and I did not figure out why.

🤦 These life cycle functions are so convoluted. From pmndrs/react-three-fiber#3549 (comment), I was under the impression that useLayoutEffect would always run initially - though I know useMemo probably runs synchronously, instead. If we use useLayoutEffect rather than useEffect do we still have these issues? The reason I switched to useLayoutEffect in the first place was because it seemed to be the case that useEffect could be delayed where layout effect was not.

It doesn't look like there are any other "effect"-style hooks we could try using to adjust the life cycle, is that right?

I think some dependencies still ask for @react-three/fiber v8 so I did not change it.

Thanks I'll take a look at this another time.

[AlaricBaraou]

These life cycle functions are so convoluted.

I'll definitely agree to that 😅

I'll try to explain the differences.

React hooks like useEffect and useLayoutEffect both run after the body of the function ( React component ) has been executed while useMemo runs immediately and its result is available immediately within the body of the function during render.

The difference between useLayoutEffect and useEffect is indeed that useLayoutEffect runs immediatly, but still after the component did render.
So if you want to pass down to children component the "result" of a useLayoutEffect, you will need one more render + the use of useState.

In the most recent commit I reverted most of my changes to handle the logic of registering / unregistering directly in the body of the component instead of relying on effects.
This way we don't rely on react hooks and don't have an issue with StrictMode call the effects twice.

This fix the issue with plugins.

For the main Tiles instance and the support of StrictMode, we needed a way to call .dispose() only when a component fully unmounts.
I found out that r3f internally call .dispose() only when a component is fully unmounted or when the arguments changes.
Using this, I was able to call tiles.dispose() only when a new instance is created or when TilesRenderer is unmounted.
I added an example /r3f/dispose.html to test this.

It's not very elegant but it works.

The alternative and maybe more "r3f way", would be to use r3f extend() and let r3f manage the dispose.
But this might require some changes on TilesRenderer to be doable.
https://r3f.docs.pmnd.rs/api/typescript#extending-threeelements

As a user, it fits my needs and fix the issues with StrictMode without causing any breaking change.
Let me know if you want me to make adjustments.

[gkjohnson]

@AlaricBaraou - this should be fixed now. Take a look and let me know what you think. The changes boil down to not relying on useMemo anymore for things that have life cycle functions like dispose (TileRenderer, Plugins) and instead using useEffect. The contexts and children of the tiles and plugins are then not fully rendered until the class instances are fully instantiated to avoid cases where "null" is passed down as a context. It seems that use strict will otherwise rerun all effect hooks in sequence when simulating unmount and remounts so the execution is reliable.

I've also fixed an issue with CanvasDOMOverlay resulting in console errors:

You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it.

edit: if it all looks good I'll probably get rid of the "dispose" example, as well, unless you think it's needed

[AlaricBaraou]

@gkjohnson

Awesome, today I learned with your changes that StrictMode only runs the effect setup and cleanup functions twice during the component's initial mount. I thought they ran twice each time, which make your solution viable indeed.

edit: if it all looks good I'll probably get rid of the "dispose" example, as well, unless you think it's needed

I understand and it's weird to have an example specific to R3F but in this instance it's the example that allowed me to find an issue in the lifecycle management of TilesPlugin.

Previous to 617ec11 when changing a tile instance, the TilesPlugin effect to register the plugin would run before the new plugin instance is created. By adjusting the effect dependency, it will now run only when a new instance is created.

I also thought about adding tiles.dispose() in TilesRenderer cleanup effect, it works fine but I leave it up to you if you think it's needed or not to call it when the component unmounts.

useEffect( () => {

		const needsRender = () => invalidate();

		const tiles = new TilesRendererImpl( url );
		tiles.addEventListener( 'needs-render', needsRender );
		tiles.addEventListener( 'needs-update', needsRender );
		setTiles( tiles );

		return () => {

			tiles.removeEventListener( 'needs-render', needsRender );
			tiles.removeEventListener( 'needs-update', needsRender );
			// tiles.dispose()
			setTiles( null );

		};

}, [ url, invalidate ] );

I suspect one potential bug in `TilesPlugin``thought.
In an example like r3f/dispose.html, when we update the tiles, TilesPlugin will re-render a first time with the previous plugin instance associated to the Tiles that just got replaced and the new Tiles instance.
Which mean that children using TilesPluginContext will have one render cycle with a plugin associated to a previous Tiles instance and the new Tiles instance.
It doesn't seem to be an issue right now but I assume it could be one.

A potential solution could be to replace this safeguard

// only render out the plugin once the instance and context are ready
if ( ! instance ) {

		return;

}

With

if ( ! instance || ! tiles.plugins.includes(instance) ) {

		return;

}

This would cause any children of the plugin to unmount but it guarantees that the children will only run with a valid Tiles + Plugin pair.

I tested both with and without and both work fine with the current examples.

[gkjohnson]

I understand and it's weird to have an example specific to R3F but in this instance it's the example that allowed me to find an issue in the lifecycle management of TilesPlugin.

I see this is working by swapping tile set urls. Is it possible we can add a test for something like this? Otherwise we can keep it for now but maybe it makes more sense to integrate this into another demo. I just want to keep the number of demos to maintain to a minimum.

Previous to 617ec11 when changing a tile instance, the TilesPlugin effect to register the plugin would run before the new plugin instance is created. By adjusting the effect dependency, it will now run only when a new instance is created.

This looks like a good change - we should just add a comment describing why "tiles" is not included in the dependencies array.

I also thought about adding tiles.dispose() in TilesRenderer cleanup effect, it works fine but I leave it up to you if you think it's needed or not to call it when the component unmounts.

Thanks for the catch - this is required. It looks like I forgot to readd the dispose call after migrating from the previous r3f group dispose approach.

TilesPlugin will re-render a first time with the previous plugin instance associated to the Tiles that just got replaced and the new Tiles instance.
Which mean that children using TilesPluginContext will have one render cycle with a plugin associated to a previous Tiles instance and the new Tiles instance.

Interesting. I'm not exactly sure how we would be supposed to work around this in react, which is annoying. For some of hte current plugin children this should be okay but why don't we add your check. I don't love accessing tile.plugins directly, though - I'll look into adding some other helper functions after this is merged to make this kind of check more simple.

[AlaricBaraou]

Interesting. I'm not exactly sure how we would be supposed to work around this in react, which is annoying. For some of hte current plugin children this should be okay but why don't we add your check. I don't love accessing tile.plugins directly, though - I'll look into adding some other helper functions after this is merged to make this kind of check more simple.

Actually my suggestion was wrong, because of this check the component can be left in a state where it will not render the children even after being registered, because we don't update the component state when registering the plugin.

I think the best thing to do is to register the plugin when it is created in the same useEffect.
The only "blocker" is the useDeepOptions that need to be applied before we apply the plugin.

If the options are meant to be applied only once on creation before registering, then it's just about moving the content of the useDeepOptions effect in a function like we did earlier in this PR.

If it's meant to be editable, with the current ability to restore the previous state when an option change, it's more tricky.