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

Missing check for promise function together with the inputs #8

Open
namnm opened this issue Feb 12, 2020 · 5 comments
Open

Missing check for promise function together with the inputs #8

namnm opened this issue Feb 12, 2020 · 5 comments

Comments

@namnm
Copy link

namnm commented Feb 12, 2020

We need to improve the if statement in the cache check to: if (promiseCache.promiseFn === promise && deepEqual(inputs, promiseCache.inputs))
Example of an invalid case:

const fn1 = () => { ... }
const fn2 = () => { ... }
// Because both functions have the same parameter
//  so the deepEqual can not guarantee the correct cache

To do so, when construct the cache item, you need to save the promiseFn as well:

  const promiseCache: PromiseCache = {
    promiseFn: promise,
    promise:
      ...
  };
@ngocdaothanh
Copy link

ngocdaothanh commented Feb 6, 2021

save the promiseFn as well...

I think we can't use promiseFn as part of the cache key, because:

  • promiseFn can be created dynamically, different instances of the same function won't be equal. For example, (() => 1) === (() => 1) returns false.
  • promiseFn.toString() is good enough, but we can't rely on it 100%, because different functions may have the same toString() result.

One solution is designing usePromise to have a category parameter (which will be included to cache key), like this:

const usePromise = (
  promise: (...inputs: any) => any, 
  inputs: Array<any>, 
  category: string = promise.toString(), 
  lifespan: number = 0
)

category is set to promise.toString() by default because it's good enough.

@ngocdaothanh
Copy link

For now, I'm using this wrapper in my projects:

import originalUsePromise from 'react-promise-suspense'

type Options = {
  category?: string
  lifespan?: number
}

export default function usePromise<T>(
  promise: (...inputs: any[]) => Promise<T>,
  inputs: any[],
  options: Options = {}
): T {
  const category = options.category || promise.toString()
  const lifespan = options.lifespan || 0

  const cacheKeys = [category, ...inputs]
  const callPromise = () => promise.apply(null, inputs)
  return originalUsePromise(callPromise, cacheKeys, lifespan)
}

@namnm
Copy link
Author

namnm commented Feb 7, 2021

@ngocdaothanh Thanks for giving your snippet, it is really insightful.

You are right we should not compare the function reference (I was proposing that because at that time most of my functions were declared outside of render function and thus they were static)

However this library is not in maintain and has other issues as well which the author doesnt seem to have interest on. So, do you think that you would like to fork the repo and publish a new package? I'm glad to support and use it in my projects.

@ngocdaothanh
Copy link

ngocdaothanh commented Feb 7, 2021

@namnm From the issues you created at this repo, it seems you are very passionate about this project, and you have more than a year of experience with this project, more than me. So please fork the repo and publish a new package.

@msereniti
Copy link

msereniti commented Jan 20, 2022

You can use fork of this package that solves this issues and many others react-use-await

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