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

render throws Promise for parameterized lazy components #55

Open
dgp1130 opened this issue Sep 5, 2023 · 6 comments
Open

render throws Promise for parameterized lazy components #55

dgp1130 opened this issue Sep 5, 2023 · 6 comments

Comments

@dgp1130
Copy link

dgp1130 commented Sep 5, 2023

I've been experimenting with Preact SSR and Suspense and tried to use preact-ssr-prepass but found render would always throw a Promise. I minified my test case and discovered this has to do with precise usage of lazy. I have a complete minimal repro here.

Essentially, this repo makes three attempts at defining a lazy component evaluated with Suspense. The first attempt looks like this:

function LazyComponent(): VNode {
    return <div>Hello, World!</div>;
}

const Comp = lazy(async () => LazyComponent);

// render as `<Suspense fallback={undefined}><Comp /></Suspense>`.

This attempt renders as you would expect, but also is kind of unnecessary. Let's get a little more complex with attempt 2.

function LazyComponent2(): VNode {
    const Comp2 = lazy(async () => {
        return () => <span>Hello, World!</span>;
    });

    return <Comp2 />;
}

// Render as `<Suspense fallback={undefined}><LazyComponent2 /></Suspense>`.

In this attempt we've moved the lazy call inside the component function to provide a bit more encapsulation. This attempt fails at render time and throws a Promise object directly with no error message. Not sure exactly what's wrong with this pattern, but clearly putting lazy inside the function breaks it. Maybe lazy can't be called at render time?

Let's try attempt 3, which is really just a justification for why you'd want to do this in the first place:

function ParameterizedLazyComponent({ id }: { id: number }): VNode {
    const Comp3 = lazy(async () => {
        const name = await getNameById(id); // Call an async function with a prop value.
        return () => <div>Hello, {name}!</div>;
    });

    return <Comp3 />;
}

// Does some async work, exactly what is not important here.
async function getNameById(id: number): Promise<string> {
    await new Promise<void>((resolve) => {
        setTimeout(resolve, 100);
    });

    return `Name #${id}`;
}

// Render as `<Suspense fallback={undefined}><ParameterizedLazyComponent id={1} /></Suspense>`.

This is the same as attempt 2, except it actually does some meaningful async work. This also fails with the same thrown Promise. Ultimately this is really what I want to do, invoke an async operation with a parameter which comes from a function prop. The only way I can see of to do this is to move the lazy call inside the component so it closes over the prop. However this pattern appears to break preact-ssr-prepass. lazy doesn't appear to provide any arguments to its callback, so I don't see any other obvious way of getting component prop data into the async operation.

I am new to Preact (and not terribly familiar with the React ecosystem in general) so apologies if this has a well-known answer. This feels like a rendering bug given that it is throwing a Promise without any error message. If there's a different pattern for developing parameterized lazy components, please let me know. As it stands, I don't see an obvious workaround here which does what I need it to do.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Sep 5, 2023

Basically what you are doing here return () => <div>Hello, {name}!</div>; is referred to as component trashing. The reference to the component will always be new, if we for instance do the following at the top-level (not inside a component hence similarly you will call lazy on every render and trash the reference)

const NameComponent lazy(async () => ('./x.js'));

In the above lazy is used to code-split out the NameComponent so this will optimise your bundle. When you want to use a render-as-you-fetch approach as you are doing in the above you can do something along the likes of

const promiseCache = {};

const App = ({ id }) => {
  const [name, setName] = useState(() => {
    if (!promiseCache[id]) {
      return (promiseCache[id] = getNameById(id).then(name => {
        delete promiseCache[id];
        setName(name);
      }))
    } else {
      return promiseCache[id].then(name => {
        setName(name);
      })
    }
  })

  if (name.then) throw name;

  return <NameComponent name={name} />
}

So with lazy you can lazy-load components so they don't add bundle-size, this can be handy for routes/... a little example from my blog, what lazy does behind the hood is what I added within the function body of App it creates a Promise and throws it to the closest Suspense boundary which will render the fallback until the promise completes. The promiseCache is intended so we have an append-only cache of results that can be shared among components so we can resolve these boundaries at the same time.

I hope this helps!

EDIT: with signals I have created a wrapper in the past that is in RFC, in case you want to try something like that https://github.com/preactjs/signals/compare/async-resource?expand=1

EDIT2: I also made a small demo in the past how this approach can help Preact look at resumed hydration because it can hydrate up until a thrown promise and resume after. The useApi part abstracts that latter throw/fetch mechanism

@developit
Copy link
Member

@JoviDeCroock's answer is the correct one - this isn't really what lazy() is for. You can contort it into the right shape using a cache if you want:

const cache = new Map();
function cachedLazy(load, parameters = []) {
  const cacheKey = parameters.join('\n'); // or JSON.stringify or whatever you prefer

  const cached = cache.get(cacheKey);
  if (cached) return;

  const comp = lazy(() => load(...parameters));
  cache.set(cacheKey, comp);
  return comp;
}

Usage:

function ParameterizedLazyComponent({ id }: { id: number }): VNode {
    const Comp3 = cachedLazy(async (id) => {
        const name = await getNameById(id); // Call an async function with a prop value.
        return () => <div>Hello, {name}!</div>;
    }, [id]);

    return <Comp3 />;
}

@dgp1130
Copy link
Author

dgp1130 commented Sep 6, 2023

Thank you both for the quick response. I think I'm understanding now. I didn't realize my tree was getting rendered twice from preact-ssr-prepass and preact-render-to-string together and required component references to be stable.

Is lazy only intended for lazy loading components, not their data? Should it only be used for the documented case of:

const Comp = lazy(async () => await import('./my-component.js'));

I get the impression I'm using the wrong primitive for what I want, but I'm not seeing an obvious alternative.

I was able to get something working with a modified form of your cachedLazy suggestion. Although I'm a little worried this will leak memory given that components are never removed the cache. I suppose they could be removed on the second read, assuming that a component will always be rendered exactly twice (not sure if that's a safe assumption to make). I feel like that might still leak memory if a component threw an error on the second render and blocked other components from being looked up in the cache and removed? Would this approach be safe for a long-lived server or a program which renders a large number of async components?

const cache = new Map();
function cachedLazy(load, parameters = []) {
  const cacheKey = parameters.join('\n'); // or JSON.stringify or whatever you prefer

  const cached = cache.get(cacheKey);
  if (cached) {
    cache.delete(cacheKey); // Remove from the cache when read.
    return cached;
  }

  const comp = lazy(() => load(...parameters));
  cache.set(cacheKey, comp);
  return comp;
}

Not sure how useful or actionable this issue is, feel free to close if it isn't. My immediate thought of potential outcomes from this would be:

  • Treat this as a feature request for a better means of parameterized lazy components.
  • Update documentation to better clarify how to handle this use case (or the limitations of lazy itself).
  • Update preact-ssr-prepass or preact-render-to-string to give a meaningful error message when making this kind of mistake.
    • As someone unfamiliar with React/Preact I was really confused when I was seeing the render call throw a Promise. I think I heard at one point that Suspense literally throws a Promise for async work, but managed to forget it.
    • This might be tricky in practice, but I wonder if we could have a global error handler somewhere to catch this Promise and convert it into a meaningful error message before emitting to the user?

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Sep 6, 2023

I encourage you to re-read my message, your use-case is most definitely supported but it's an in-render process.

preact-ssr-prepass will traverse the tree until no more promises are thrown hence you can combine loading lazy and an in-render promise for data.

My example is simplified but you can create a top-level PromiseCache by doing for instance:

const render = () => {
  prepass(
    <CacheProvider><App />
  )
}

especially the useApi part caters to that where the cache is provided only for pre-rendering. This is a data-cache and all the components themselves are lazily loaded.

@dgp1130
Copy link
Author

dgp1130 commented Sep 7, 2023

Ah ok, IIUC, you're saying I really shouldn't be using lazy at all and instead throw the Promise to await the task and use the cache to pick up where it left off. To rewrite this in my own style and confirm my understanding:

const map = new Map<number, string>();

function ParameterizedLazyComponent({ id }: { id: number }): VNode {
    const name = map.get(id);

    // If we didn't previously cache a name, then we need to do the async work to get it.
    // We throw the `Promise` so the nearest `<Suspense />` waits and rerenders when it is resolved.
    // On subsequent renders, `name` is found.
    if (!name) {
        throw getNameById(id).then((name) => { map.set(id, name); });
    }

    return <div>Hello, {name}!</div>;
}

Am I following this correctly? Is there any documentation on this throw Promise pattern? Trying to search more about this, I was surprised to find that there don't seem to be common patterns for doing this kind of thing. React's documentation seems to handwave a lot of this complexity away and talk about "Suspense-enabled data fetching libraries". Does that just mean they throw Promise objects?

While this seems to work, I noticed that your approach is caching the Promise directly which could be necessary if the component is rendered twice before the Promise resolves. Is that possible? I'm not seeing that happen in my simple case, but maybe more complex component trees can do that?

I tried an alternative approach with useState which felt more idiomatic:

function ParameterizedLazyComponent({ id }: { id: number }): VNode {
    const [ name, setName ] = useState<string | undefined>();

    if (!name) {
        throw getNameById(id).then((n) => { setName(n); });
    }

    return <div>Hello, {name}!</div>;
}

Unfortunately this doesn't actually work. The component renders three times (two from preact-ssr-prepass, and the third from preact-render-to-string). The first render initializes the state, the second reads it, but then the third does not retrieve the preexisting state and calls getNamedById again. I'm guessing that's working as intended since these are two different packages which don't inherently share state outside the VDom tree.

Regarding the memory leak, I also tried deleting the item from the cache after it is read, but that doesn't work because there are three renders, not two. Deleting the cache item after the first read breaks the third render. I suspect the number of renders wouldn't be consistent anyways due to a varying number and structure of other components suspending. So I'm still not clear on how to deal with the memory leak there?

To back up a bit, my actual objective is a static-site generator with Preact/JSX as the templating engine (called @rules_prerender). My goals here are two fold:

  1. Do some async work in the documentation site. In this case, it is reading a markdown file and converting it to HTML to embed inside a Preact component, which I was hoping to use <Suspense /> for.
  2. Figure out common patterns for users of my SSG toolchain to leverage for their own async work.

It feels like maybe the better approach is to do a fetch-then-render architecture so I avoid the <Suspense /> altogether? It's certainly feasible for my use case and might make more sense than <Suspense /> in general. Particularly given that fallback content is kind of meaningless in this context, I always want to wait for the fully rendered page.

Based on Preact's documentation, React's note that data fetching is not really supported with Suspense, and your own blog post it seems like data fetching with <Suspense /> isn't a very well-trodden path at the moment? I would love to do a fetch-as-you-render approach which seemed to be the objective with <Suspense /> and would have the ideal performance for SSR/SSG and potentially work with streaming, but it seems like this isn't really feasible today?

Sorry, I'm starting to ramble and talk about higher-level problems I need to solve for @rules_prerender. Just trying to give some context if that's helpful at all. Feel free to close this issue since "parameterized lazy components" doesn't really seem like the right mechanism for solving the more general async data loading problem I actually have.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Sep 7, 2023

Well if you want to make this into a reusable piece it is most definitely possible, i.e. like the linked useApi I had in both previous messages you can provide something like that to your consumers. This can facilitate the whole render-as-you-fetch during the server.

Yes, your component is execute multiple times...

Preact-ssr-prepass executes the components until no more promises throw

There is a different option here where you for instance allow your users to provide a function that asynchronously gets the data, some loader-like pattern as Suspense for me is more for SPA-like applications.

Data-fetching libraries like urql indeed implement throwing Promises, I think one of the personal reasons why I have not written a lot about Suspense would be that it's a compat feature and there should be better primitives that I have yet to figure out. Like the asyncComputed I linked in my other message.

I feel your pain here though hence I am very open to ideate with you.

Btw, have you checked out fresh might be a good source of inspiration as well

dgp1130 added a commit to dgp1130/rules_prerender that referenced this issue Sep 8, 2023
Refs #16.

This got a little involved, but I learned a lot in the process here. The general pipeline is:
1.  Perform an `async` read of the `getting_started.md` file.
2.  Parse the markdown with `gray-matter` to separate the frontmatter and markdown body.
3.  Parse the markdown body with `marked` and transform it into HTML.
4.  Validate the frontmatter with a `zod` schema.
5.  Render the page in Preact.

This relies on a "fetch-then-render" approach, where all the data is loaded up front asynchronously _and then_ rendered afterwards. I initially tried a "fetch-as-you-render" approach with `<Suspense />` but found it to be a bit lacking. Preact's `<Suspense />` implementation is still experimental, and currently intended for lazy loading components. Data loading with `<Suspense />` is not really a well-trodden path and does not have defined primitives, either in Preact or React. Most React frameworks seem to define their own data loading primitives which I was initially hoping to avoid, letting users structure their own rendering process.

For now, the easiest option is to fetch data up front and render afterwards. This isn't great performance and I'm not totally satisfied with it, but it's good enough for now. In the future, hopefully we can revisit async Preact and come away with a better path forward.

[See discussion in `preact-ssr-prepass`.](preactjs/preact-ssr-prepass#55)
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

No branches or pull requests

3 participants