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

Bug: useAssets might attempt state updates before mounting #566

Open
Tracked by #493
RJWadley opened this issue Jan 10, 2025 · 6 comments
Open
Tracked by #493

Bug: useAssets might attempt state updates before mounting #566

RJWadley opened this issue Jan 10, 2025 · 6 comments
Assignees
Labels
v8 Issues related to Pixi React v8

Comments

@RJWadley
Copy link

Current Behavior

if the render is suspended after useAssets is called but before the image finishes loading, we get an error:

Image

Expected Behavior

useAssets should not cause errors if rendering is suspended

Steps to Reproduce

the easiest way to reproduce this is to create a component that suspends:

const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));

export function AssetTest() {
  const {
    assets: [texture],
    isSuccess,
  } = useAssets<Texture>([
    "https://fastly.picsum.photos/id/436/4000/4000.jpg?hmac=wzOFg9pak_9vNY9IF9Bz43cwQXdtmOGqXiWS-F9IzpY",
  ]);

  throw sleep(100);
  return null;
}

I've also prepared a codesandbox that throws this error. Note that you'll want to open the preview in a new tab because the codesandbox preview frame causes extra unrelated errors.

Environment

Possible Solution

  • the actual setState should probably go into an effect
  • in this case it's probably fine to start fetching the asset during the render - just don't update any react state
  • if the asset is already cached, it's fine to return it on the first render and skip the effect work later

Additional Information

No response

@trezy trezy added the v8 Issues related to Pixi React v8 label Jan 16, 2025
@thejustinwalsh
Copy link
Collaborator

thejustinwalsh commented Jan 20, 2025

@RJWadley thanks for catching this, I exclusivly have been using useSuspenseAssets with suspense, and never thought to run useAssets through a suspense test...

With React 19 out officially, I think we need to take a pass on simplifying the hooks and leaning on the new use hook. I don't beleive there needs to be a difference between the two hooks anymore.

@trezy trezy self-assigned this Jan 28, 2025
@trezy trezy mentioned this issue Jan 28, 2025
@thejustinwalsh
Copy link
Collaborator

thejustinwalsh commented Jan 30, 2025

@RJWadley I am working on a react 19 versiond of these hooks, and came up with something like this...
Let me know if you see any issues, but this should work in all cases and optionally support suspense with the use hook.

https://gist.github.com/thejustinwalsh/d2d46e4498de398e7c0390aa5b167fd5

import {use, useEffect, useState} from 'react';
import {Assets} from 'pixi.js';

import type {UnresolvedAsset} from 'pixi.js';

export type SynchronousState = {
  isLoading: boolean;
  error: Error | null;
};

export function useAssets<T>(urls: string | UnresolvedAsset): SynchronousState & {data: T | undefined};
export function useAssets<T>(urls: string[] | UnresolvedAsset[]): SynchronousState & {data: Record<string, T>};
export function useAssets<T>(urls: string | UnresolvedAsset, suspend: true): T;
export function useAssets<T>(urls: string[] | UnresolvedAsset[], suspend: true): Record<string, T>;

export function useAssets<T>(urls: string | UnresolvedAsset | string[] | UnresolvedAsset[], suspend = false) {
  const [state, setState] = useState<SynchronousState & {data: T | Record<string, T> | undefined}>(() => ({
    isLoading: suspend ? false : isLoaded(urls),
    error: null,
    data: suspend ? [] : resolve(urls),
  }));
  const [assets] = useState(() => Assets.load<T>(urls));
  useEffect(() => {
    if (!suspend) {
      assets
        .then(data => setState({isLoading: false, error: null, data}))
        .catch(error => setState({isLoading: false, error, data: undefined}));
    }
  }, [assets, suspend]);

  if (suspend) {
    return use(assets);
  }

  return state;
}

const key = (url: string | UnresolvedAsset) => (typeof url === 'string' ? url : (url.alias ?? url.src)?.toString());

const isLoaded = (urls: string | UnresolvedAsset | string[] | UnresolvedAsset[]) =>
  Array.isArray(urls) ? urls.every(url => Assets.cache.has(key(url))) : Assets.cache.has(key(urls));

const resolve = (urls: string | UnresolvedAsset | string[] | UnresolvedAsset[]) =>
  Array.isArray(urls)
    ? urls.reduce(
        (acc, url) => {
          const k = key(url);
          acc[k ?? ''] = Assets.cache.get(k);
          return acc;
        },
        {} as Record<string, unknown>,
      )
    : Assets.cache.get(key(urls));
// Without Suspense
const { isLoading, error, data: ships } = useAssets<Spritesheet>('./assets/ships/atlas.json');

const { isLoading, error, data } = useAssets<Spritesheet>(['./assets/ships/atlas.json', './assets/enemies/atlas.json']);
const ships = data?.['./assets/ships/atlas.json'];
const enemies = data?.['./assets/enemies/atlas.json'];

// With Suspense
const ships = useAssets<Spritesheet>('./assets/ships/atlas.json', true);

const {
  ['./assets/ships/atlas.json']: ships,
  ['./assets/enemies/atlas.json']: enemies,
} = useAssets<Spritesheet>(['./assets/ships/atlas.json', './assets/enemies/atlas.json'], true);

@RJWadley
Copy link
Author

@thejustinwalsh that looks like it should work, I’ll give it a test when I get home today!

one potential issue is things may become outdated if the assets change, since it looks like you’re using a singleton with useState for that:

const [assets] = useState(() => Assets.load<T>(urls));

if we were to, say, render with an empty array first and then later rerender with assets i don’t think they’d load
or if we rerendered with a different asset on the second render i don’t think it would update

I’m also not totally sure about having the second parameter as a boolean. makes it a bit unclear what the behavior is when you’re looking at the usage code. maybe an options object or a string would be more clear? or keep around useSuspenseAssets but just as an alias to useAssets(…, true)? I also might be overthinking that.

@thejustinwalsh
Copy link
Collaborator

thejustinwalsh commented Jan 30, 2025

Nice catch. We have to create the promise in the initializer, otherwise react 19 wont let us use a client side promise created in render.

Updating the state of the promise if the urls change should work, as the new promise would still be created in an effect, and not on render. This was fresh off the press of the first working edition with react 19 use hook. Thanks for the feedback, I will keep at it.

I am biased on the naming, because honestly I think suspense should be the default, and you should have to opt out of it to do conditional loading, the code differences between the suspense and non-suspense version were so close that I felt like it could really be one hook.

Naming the mode with an options object or string is cool with me, just have to figure out the types, was trivial to overload the param when true. Was trying to keep the hook API as close to the Assets.load API as I could as well.

Also, have not ran any of this through SSR just yet, this is a client only hook due to it creating renderer resources on load.

@RJWadley
Copy link
Author

RJWadley commented Jan 31, 2025

ah, gotcha. I seem to recall that the promise must be cached in order to suspend with it, which is what useState would be doing for you i think? You might be able to accomplish the same thing with useMemo? Regardless, I think it's probably fine as-is for now if that doesn't work, we'll just need to call it out in the docs.

here's the same function type, but using an options object and a more specific return type
  • uses an options object, for example { suspense: false }
  • would default to suspense true
  • when opted out of suspense, a union lets us narrow the type cleanly at usage time
type AssetsResult<T> =
    | {
            status: "loading"
            isLoading: true
            error: null
            data: undefined
      }
    | {
            status: "error"
            isLoading: false
            error: Error
            data: undefined
      }
    | {
            status: "success"
            isLoading: false
            error: null
            data: T
      }

// unsuspended version
export function useAssets<T>(
    urls: string | UnresolvedAsset,
    options: { suspend: false },
): AssetsResult<T>
export function useAssets<T>(
    urls: (string | UnresolvedAsset)[],
    options: { suspend: false },
): AssetsResult<Record<string, T>>

// suspended version
export function useAssets<T>(
    urls: string | UnresolvedAsset,
    options?: { suspend?: true },
): T
export function useAssets<T>(
    urls: (string | UnresolvedAsset)[],
    options?: { suspend?: true },
): Record<string, T>

export function useAssets<T>(
    urls: string | UnresolvedAsset | string[] | UnresolvedAsset[],
    options: { suspend?: boolean } = { suspend: true },
) {
    const suspend = options.suspend ?? true
    // ... rest of implementation
}

and here's usage:

function Example() {
    const { status, isLoading, error, data } = useAssets<Texture>("./image.png", {
        suspend: false,
    })

    if (isLoading) return null // same as if (status === "loading")
    if (error) return null // same as if (status === "error")

    return <pixiSprite texture={data} />
}

@thejustinwalsh
Copy link
Collaborator

Nice, the options object works just as well for the types! Thanks for that example!

useState is serving two purposes, caching the promise, and the initializer function is also ensuring the promise is not created at render time. Both must be true for suspense to work with the use hook.

I think I have also convinced myself that it probably should be two hooks after refactoring several times. If you allow the user to toggle suspense at runtime I either produce 1 additional render to synchronize the state if they decide to stop using suspense at runtime, or I have to ignore the suspend option after mount, and at that point, might as well make it two hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to Pixi React v8
Projects
None yet
Development

No branches or pull requests

3 participants