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

Handle primitives in test-renderer and fix queries in TestInstances #3507

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlaricBaraou
Copy link
Contributor

Handle primitives in test-renderer and fix queries in TestInstances

Description

This PR addresses two significant issues in the React Three Test Renderer:

  1. Primitive Children Not Visible: Previously, when using <primitive object={object} /> components, the children of these objects were not visible in the test renderer's output. This made it impossible to test or traverse the full scene graph when primitives were used.

  2. Incorrect Query Behavior: The search methods (findByType, etc.) were including the current node in search results, which made it difficult to search for nested elements of the same type.

Changes

Added Primitive Children Support

  • Modified createTestInstance.ts to detect primitive components and include their THREE.js children in the virtual tree
  • Added a recursive createVirtualInstance helper that creates instance representations for THREE.js objects
  • Updated graph.ts to properly include primitive children in the scene graph output

Fixed Query Methods

  • Changed the default search behavior to exclude the current node when searching

Added tests

  • Added test cases for searching through nested primitives
  • Added tests to ensure graph output correctly shows primitive hierarchies
  • Added regression tests for the fixed query behavior

Other improvements

  • Fixed imports in the documentation
  • Removed reference to non-existent folder in jest config

Note on implementation

I've tried to keep the implementation minimal to address just the specific issues. For the createVirtualInstance function, I couldn't directly import the prepare method from fiber's utils.tsx, so I created a simplified version that follows the same pattern.

I'm happy to make any modification you could suggest to improve the PR if needed.

Copy link

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

Sandbox Source
example Configuration

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Mar 25, 2025

I think primitive children is a design question for @joshuaellis. I would normally say the current behavior works as intended (even if not useful) but the lines have been muddy here with incorrect language. There is a distinction between the actual JSX and underlying scene and I can't think of that being useful. Just a question of how to rectify this from a design perspective as this is not exhaustive.

@AlaricBaraou
Copy link
Contributor Author

AlaricBaraou commented Mar 26, 2025

I see, would love to hear what @joshuaellis thinks then.

In my case I was testing a component that is using a primitive to render a glb, that same primitive fallback to a different native three.js object in different scenarios and being able to query the content of the primitive would help testing that a lot ( I'm sure I could find a workaround too that don't rely on this feature though ).

But the second part of this PR about querying from a lower node seems important to fix, the fact that currently the node from which you search is included in the result is very confusing and limiting.

I could split the PR in two to address the two issues separately. What do you think ?

@CodyJasonBennett
Copy link
Member

Agreed and sure. I'm traveling now but I'll get to this once I can or have another maintainer do it.

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