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

(v4) Tests without root fail #551

Closed
5 tasks done
andretchen0 opened this issue Feb 11, 2024 · 2 comments
Closed
5 tasks done

(v4) Tests without root fail #551

andretchen0 opened this issue Feb 11, 2024 · 2 comments

Comments

@andretchen0
Copy link
Contributor

andretchen0 commented Feb 11, 2024

Describe the bug

Description

I'm trying to write some simple vitest tests for nodeOps. Here's an example:

  it ('should not throw on nodeOps.remove', async() => {
    const mesh = nodeOps.createElement('TresMesh')
    const parent = nodeOps.createElement('TresMesh')
    nodeOps.insert(mesh, parent)
    nodeOps.remove(mesh)
  })

This test fails with the following error:

TypeError: deregisterBlockingObjectAtPointerEventHandler is not a function

Desired outcome

Insertions/removals without a root shouldn't throw.

Steps to reproduce

  • Insert example into src/core/nodeOps.test.ts
  • pnpm run test:ui
  • See failing test

Context

Related: #515

The code that throws starts at line 137:

    const ctx = node.__tres
    // remove is only called on the node being removed and not on child nodes.
    node.parent = node.parent || scene
    
    const { 
      deregisterObjectAtPointerEventHandler,
      deregisterBlockingObjectAtPointerEventHandler, 
    } = ctx.root

Proposed solution

Dispatch an event and handle at the root. That would allow the tests to pass and reduce some coupling at the same time.

For what it's worth, this is how THREE.Material.dispose() works, which is called in the Tres code right below the block shown above.

Used Package Manager

pnpm

Code of Conduct

@alvarosabu
Copy link
Member

Hi @andretchen0 I think @garrlker PR #529 will remove the need to use those methods. But, for tests we will need to mock the root state since its widely used

@andretchen0
Copy link
Contributor Author

@alvarosabu Ok. I'll close the issue here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants