pmndrs/suspend-react

🚥 Async/await for React components

suspend-react's Issues

PeerDependencies complaint under React 16.14

Project compatibility is listed as all React versions >= 16.6, but has a peerDependency of "react": ">=17.0"

This complains in React 16.x projects, though the library works fine.

Source of 0.0.10

Hello, I saw via renovate that the npm package has been upgraded to 0.0.10 but the repo shows 0.0.9 with the latest change from 2022.

https://app.renovatebot.com/package-diff?name=suspend-react&from=0.0.9&to=0.0.10

Is it not being developed in a branch of this repo?

Assets are not cached correctly using array as useLoader input

• three version: 0.149.0
• @react-three/fiber version: 8.10.1

Problem description:

By using Array as input in drei/useTexture or drei/useGLTF, the files are not cached correctly. As you can see, assets are logged twice in the console

Relevant code:

https://codesandbox.io/s/drei-loader-cache-issue-qce40y

Suggested solution:

Error probably comes from useLoader

Wrong file ending.

Package.json has "main": "dist/index.cjs", but package has dist/index.cjs.js which breaks the metro bundler.

We can either update the package content or rename the file?

How to clean ressources ?

Any way to clean resources using this library. I would like to open a socket using suspense but when the Host change, i need to reload a new socket and would like to clean effect the previous.

Cancellation

The ask here is:

• suspend accepts a first parameter that is an AbortSignal.
• when the component is unmounted - abort the controller associated with the signal.

e.g.

function Post({ id, version }) {
  const data = suspend(async ({ signal }) => {
    const res = await fetch(`https://hacker-news.firebaseio.com/${version}/item/${id}.json`, { signal })
    return await res.json()
  }, [id, version])
  return (
    <div>
      {data.title} by {data.by}
    </div>
  )
}

Basically allowing aborting expensive async actions.

TypeScript Improvements

I think there is some room for improvement

1. clear and peek are essentially "untyped".

   Imagine I'm using this library for fetching two resources A and B, A has dependencies as [number, number] and B has dependencies [number, string]. So I need a way to make clear(["foo", "bar"]) not compile.

   In other words I need to pass the type of (global) dependencies (which is [number, number] | [number, string] in this case) to the library. So here are the ways to do it...

   a. Tell users to pass the types via extending an interface via module augmentation

   // @file my-project/src/suspend-react-types.d.ts
   import "suspend-react";
   declare module "suspend-react" {
     export interface Types {
       dependency: [number, number] | [number, string]
       // should be named as "key" see the suggestion on nomenclature at the end
     }
   }
   
   // @file my-project/src/Foo.tsx
   import { clear } from "suspend-react";
   
   clear(["foo", "foo"]) // won't compile

   If the user doesn't provide the types we'd fallback to unknown[] for dependency.

   b. Change the API.

   // @file my-project/src/extras.ts
   import { createSuspender } from "suspend-react";
   
   const suspender = createSuspender<[number, number] | [number, string]>();
   // does nothing special in runtime just exports the functions with proper types
   export default suspender;
   
   // @file my-project/src/Foo.tsx;
   import { clear } from "./extras";
   
   clear(["foo", "foo"]) // won't compile

   c. Add an extra type-safe API.
   Same as above except we'd also export the current exports (suspend, clear, etc) along with createSuspender so the folks who don't care about types can directly use suspend, clear, etc without having to use createSuspender

   d. Don't bother about it, it's not a problem.
   Right now you can kinda achieve type-safety like this...

   // @file my-project/src/extras.ts
   export type MyDependency = [number, number] | [string, string]
   
   // @file my-project/src/Foo.tsx;
   import { clear } from "suspend-react";
   import { MyDependency } from "./extras";
   
   clear<MyDependency>(["foo", "foo"]) // won't compile

   But now you can pass you can pass whatever type different from the actual dependency, you can forget to pass it, you can pass one type to clear but other type to the second arg of suspend, etc xD

   My recommendation would be to go with a or d

2. suspend signature can be improved

   This is not much of a big deal just that right now if you hover over suspend this is what you see...

   

   We can change the type a bit so that the user sees this instead...

   

   Or even this...

   

   Though these both come with a negligible risk, here's what Ryan said about it... (although the issue he's commenting on is different but I think it still applies)

   Just an update - T & { } creates a "lower-priority" inference site for T by design. I would move this from the "definitely don't depend on this" column to the "it's probably going to work for the foreseeable future" column.

   I think we should keep it as it is I guess there's no much gain for the however small risk incurred, just giving the options anyway :P

Let me know if you like any of these improvements, then I'd send a PR. Or if you're fine as the way things are (no problems in that too ofc) you can close this issue.

Also, I'd suggest to change the nomenclature of "dependencies"/"dependency"/"args" to "key" (singular) as the tuple is the key for the cache, it just happens to be dependencies of the suspend function but other than that it doesn't make sense to name the argument of clear as "dependencies" or "args". And also you'd want to come up with a name for the function that one passes in suspend (right now it says "fn"), maybe "resolver"?
It's important to get the nomenclature right because even though it doesn't get reflect in the API anywhere, it does in the documentation, types, source, etc.

Contribution Guide

I wrote some tests for this library and how it interacts with React suspense. Is there any guidelines/process for contributing those to the project? I also have some ideas for enhancements I'd like to create and contribute.

Discussion: Concerns about uncollectable references?

Thanks for publishing this, I wrote something quite similar recently and it's been inspiring to read other similar implementations.

For my use case I have some top-level context providers that I suspend while they fill their context with essential data for the rest of the app. In my own implementation I ended up using a key (that I name appropriately in the code) to retain the various suspended states.

I was curious how the "dependencies array" concept worked in suspend-react (and learned about fast-deep-equal, thanks!) and in the process noticed that new entries are pushed into the cache, when dependencies change, but unless clear is called manually (with the old dependencies) the cached item is retained.

My question is: Is hanging onto the old promises (and in turn the reference to their result) something I should be concerned about? Correctly calling clean when the dependencies change seems challenging.

Suggestion: Document ways to avoid key collisions

Imagine if you have two resolvers getPost: (id: number) => Promise<Post> and getComment: (id: number) => Promise<Comment> but they both have same cache, that means if you fetch a post with id 1 then it gets stored in the cache with key [1] and then if you fetch a comment with id 1 it would return the post cached instead of the comment (!!!).

It's pretty simple to solve this problem you'd just add an identifier to the key...

- suspend(getPost, [id])
+ suspend(getPost, [id, "post"])

- suspend(getComment, [id])
+ suspend(getComment, [id, "comment"])

And now we're no longer only using the id as key but also an unique identifier (The correct way to solve this would have been to add the resolver function reference to key but then it won't work with anonymous functions). This is the gist although there are many ways to abstract over it. For example...

1. One could use a random unique id generator (perhaps even React.useId() would work if you don't want to use clear, peak, etc outside the component)

   // @file src/extras.ts
   let lastResourceId = -1;
   export const createResourceId = () => ++lastResourceId;
   
   // @file src/Post.tsx
   import { suspend } from "suspend-react";
   import { createResourceId } from "./extras";
   import { getPost } from "./api.ts";
   
   const POST_RESOURCE = createResourceId();
   export default function Post({ id }: { id: number }) {
     let post = suspend(getPost, [id, POST_RESOURCE]);
   }

2. Write a wrapper that takes the resolver

   // @file src/extras.ts
   import { suspend, clear, peak } from "suspend-react";
   let lastResourceId = -1;
   export const createResource = <K, T>(resolver: (...key: K) => T) => {
     let id = ++lastResourceId;
     return ({
       suspend: (key: K) => suspend(resolver, [...key, id]),
       clear: (key: K) => clear([....key, id]),
       peak: (key: K) => peak([...key, id])
     })
   }
   
   // @file src/Post.tsx
   import { createResource } from "./extras.ts";
   import { getPost } from "./api.ts";
   
   const postResource = createResource(getPost) // K and T both get inferred
   export default function Post({ id }: { id: number }) {
     let post = postResource.suspend([id]);
   }

One caveat is that, one can't clear all entries only for a specific identifier. If there was an api where clear takes a predicate, that would make this possible...

- clear: (key: K) => clear([....key, id])
+ clear: (key?: K) => k !== undefined ? clear([....key, id]) : clear(k => k.at(-1) === id)

Note that one doesn't have to worry about this whole issue at all if they're sure that there will be no collisions at all (which I guess would be common as most of the times identifiers are usually random instead of auto-increment). I think it's important to document this, or at least mention somewhere that the cache is global. Also if you're documenting, document as little as you want and then just link this issue for users to learn more.

Another question would be should suspend-react come with some api to avoid this problem? I can't answer that because I don't know what the goals of this library are. But given you say "as simple/low-level as possible", I think a global cache is okay, users can write their higher-level wrappers is they want. And also if we introduce an api like one of the above it'd result into more boilerplate for people who don't have the key collision problem.

I don't know how React is planning it's built-in cache thing and how this would interop with that, or maybe we won't have this problem anymore as the cache would not be global, I haven't looked into things so I'm not sure.

shallowEqualArrays falling short when using empty array as key

If a key is an empty array, suspend is called in an infinite loop, due to [] !== [] in js

// lets say this returns "[]"
const invoices = someCallFromDb()
// this is never settled, infinite looped
const paymentMoraleValues = suspend(
  async () => paymentMoraleFromInvoices(invoices),
  ['paymentMoraleValues', invoices]
)

Here's a sandbox for reproduction: https://codesandbox.io/s/suspend-react-infinite-rerender-mq391t

I know one can provide custom equality functions, but should such a basic case not be covered straight out of the box?

Would be a simple as making shallowEqualArrays account for it.

Fallback to previous render

This might not be something this library can handle but I would appreciate any advice. I'm hoping to fallback to the previously rendered HTML rather than a fallback component, basically doing nothing until the async code has completed.

I realize React 18 has startTransition which can wait for the state to load, but it involves calling setState and starting a new render, which adds a frame to the rendering process as far as I am aware. I want to pause the rendering until the async code is complete, then finish it, without flickering to a fallback component.

For some more context, I have a webworker operation that runs a bunch of calculations in parallel in response to user input. It only takes 2-3ms, so I want to just pause the rendering and wait for this to complete. If I do the calculations with useLayoutEffect and setState, I will end up waiting for react to render the next frame which adds additional lag.

Re-rendering on clear

Love the simplicity of the API, I'm in the process of converting all my hand rolled suspense helpers over to this!

One piece of functionality I'd love to have in the library or at least your opinion on how to implement it separately best is forcing a re-render when the cache is cleared/updated.

How would you use suspend-react when fetching through a hook?

For example with a usePostData hook:

const Post = ({ postId }) => {
  const { title } = usePostData(postId)
  return (
  	<Text>{title}</Text>
  )
}

Any insights would be greatly appreciated, thanks!