Possible race condition problem in 1.0.0 beta about react-pixi-fiber HOT 6 CLOSED

Hey @michalochman just wondering if you had a chance to look at this. I've not been able to upgrade to 1.0.0 and I think it is because of issues similar to this one. Thanks!

from react-pixi-fiber.

Hi @zenblender, I had a brief look, but it was not obvious why it is happening. I have yet to dedicate more time to it, but since the example is contrived I didn't make it a priority.

Is this also happening for you in realistic scenarios?

from react-pixi-fiber.

I'm not totally sure @michalochman. I was actually trying to replicate a real issue I'm having with 1.0.0 beta from my full application (a game), simplified to demonstrate just that problem. In doing so, I ran across this scenario. I typically don't use state in my app, and instead use Redux and useSelector to drive the rendering of components. Whether or not the codesandbox demonstrates the exact issue I'm having, or is the only issue I'm having, it does seem like there's a problem considering it throws an error sometimes and the stable version doesn't throw any errors. Thanks!

from react-pixi-fiber.

@zenblender I found that in your example we're sometimes dealing with PIXI.DisplayObjects that are destroyed, you can easily confirm it by checking containerRef.current?._destroyed in your example.

However, the root issue is not a race condition in react-pixi-fiber, but rather in your containerRefFn that you pass to <Container ref={containerRefFn} />.

That function receives null when the container is unmounted, yet your code ignores this fact right at the start of the ref callback:

export const Animation: FC = () => {
  const containerRef = useRef<PIXI.Container>()
  const containerRefFn = useCallback((container) => {
    if (!container) {
      return
    }
    containerRef.current = container
  }, [])

  // ...

  return <Container ref={containerRefFn} />
}

then later your code is still using containerRef.current even though it should not as it is already destroyed and unmounted.

Essentially, it should rather look like this:

export const Animation: FC = () => {
  const containerRef = useRef<PIXI.Container>()
  const containerRefFn = useCallback((container) => {
    containerRef.current = container
  }, [])

  // ...

  return <Container ref={containerRefFn} />
}

And then, since your not doing anything else in your callback ref, why won't event simplify it more by just passing containerRef directly as ref in <Container ref={containerRef} />?

from react-pixi-fiber.

Thanks for taking a thorough look. I see what you're saying. I was curious as to why the stable version doesn't throw any errors, but I see that in any case, there's an issue with the logic on my end.

I will try to find time for another attempt to identify issues preventing me from upgrading to 1.0.0. Until then, I am blocked from doing some dependency upgrades.

Thanks again for your time!

from react-pixi-fiber.

I am not 100% certain but, this might be related to changes to react-reconciler or even react, not necessarily to changes to react-pixi-fiber. I would check if it was not time consuming to upgrade all the dependencies of current "stable" branch.

from react-pixi-fiber.