TypeScript Improvements

[devanshj]

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.

[drcmda]

thanks for the input, super apreciated! im going with your instincts, they seem warranted. having this finally infer types is amazing (using use-asset for 1.5 years, but typing was a major pita). i renamed args to keys and documented it accordingly.

[miguel-silva]

Similarly to what the OP mentioned in one of the approaches (1. b), I was also thinking that one of the better ways to improve type safety is to change the API in a way that enforces creating individual caches, each for a specific pair of Keys and Promise result structures, instead of relying on a global one (which doesn't prevent Keys collisions for different result structures)

@drcmda, I hope you don't mind, but to illustrate what that could look like I created an adapted version of suspend-react based on one of your demos.

Basically the adapted api there closes over the loader function and config from the args of the "suspending store" factory:

function createSuspendingStore<Keys extends any[], Value>(
    fn: (...keys: Keys) => Promise<Value>, config?: Config
  ): {
    suspend(keys: Keys): Value,
    preload(keys: Keys): void,
    peek(keys: Keys): Value,
    clear(keys?: Keys): void
  }

const postsStore = createSuspendingStore(fetchPost);

const post = postsStore.suspend([id]);

Alternatively the loader function and config could be moved back to the related methods, but consuming them involves a bit more boilerplate:

function createSuspendingStore<Keys extends any[], Value>(): {
    suspend(fn: (...keys: Keys) => Promise<Value>, keys: Keys, config?: Config): Value,
    preload(fn: (...keys: Keys) => Promise<Value>, keys: Keys, config?: Config): void,
    peek(keys: Keys): Value,
    clear(keys?: Keys): void
  }

const postsStore = createSuspendingStore<[number], HNPost>();

const post = postsStore.suspend(fetchPost, [id]);

What do you think?

[devanshj]

@drcmda My pleasure! In that case I'll send a PR implementing 1.a soon (2 is okay to be left as it is as I said)

@miguel-silva Your concern around key collisions is fair, but I think it can be solved easily userland, I opened an issue suggesting how

[drcmda]

@miguel-silva key collisions are easy to avoid, ,[key1, key2, "pmndrs/suspend-react/functionName"]) or

const uid = new Symbol()
suspend(fn, [key1, key2, uid])

i wanted this library to be as simple as humanly possible. cache store already introduce things that aren't so straight forward.

[drcmda]

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

[devanshj]

Blocked on microsoft/TypeScript#46724, waiting to see if it's a bug. If it is then will wait till it gets fixed, if it's not a bug then will see if we can do some workarounds. In worst case we can write declaration by hand instead of the making the compiler emit wrong ones.