pmndrs / suspend-react

🚥 Async/await for React components
MIT License
1.37k stars 25 forks source link

Suggestion: Document ways to avoid key collisions #5

Closed devanshj closed 2 years ago

devanshj commented 2 years ago

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.

drcmda commented 2 years ago

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.

that is a good point, react will most definitively help with such issues in the long run. the cache lives in the component graph and then users decide where keys a valid:

import { Cache } from 'react'

<Cache>
  <SuspendingComponent />

i will start with a simple addition for unique cache keys. reacts useId() will be very interesting, we could add this when it drops.

drcmda commented 2 years ago

added a small section https://github.com/pmndrs/suspend-react#making-cache-keys-unique

devanshj commented 2 years ago

Looks good