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

Pass dependencies to useMemo callback as arguments #14835

Closed
davej opened this issue Feb 12, 2019 · 4 comments
Closed

Pass dependencies to useMemo callback as arguments #14835

davej opened this issue Feb 12, 2019 · 4 comments

Comments

@davej
Copy link

davej commented Feb 12, 2019

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

The useMemo factory function does not receive any arguments.

What is the desired behavior?

The useMemo factory function would receive the dependencies as arguments.

Why?

This would allow more compact syntax for memoizing components because of implicit returns and desctructuring. This came to mind after experiencing some of the issues in #14110. There may be other potential use cases too

Example of current behavior

const Avatar = () => {
  const [src] = useSomeGlobalState([
    state => state.user.avatar.src
  ]);
  return useMemo(() => <img src={src} />, [src])
}

Example of proposed behavior

const Avatar = () => 
  useMemo(
    (src) => <img src={src} />,
    useSomeGlobalState([state => state.user.avatar.src])
  );

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.8.1

@threepointone
Copy link
Contributor

I'd argue that the former is explicit, while the latter isn't. Leaving aside that this pattern isn't recommended (we recommend hooks at the 'top' level only, much like imports), the proposal would only be useful when using an array that another function returns, whereas in practice that's a very small fraction of useMemo usages; in practice, you'd be using dependencies from different sources. It would also be confusing when people do use closures (which we recommend for all hooks) but not see updates.

I'm going to close this because I don't think we'll be doing this any time soon.

@davej
Copy link
Author

davej commented Feb 12, 2019

@threepointone Thanks for the feedback 👍

@aweary
Copy link
Contributor

aweary commented Feb 12, 2019

@threepointone this seems like a good topic for the Hooks FAQ 🙂

@remcohaszing
Copy link

I had the same idea, but I think the example of current vs proposed behaviour contains too many changes. Also my idea extends to useEffect().

current behaviour:

export function Image({ blob }) {
  const url = useMemo(() => {
    return URL.createObjectURL(blob)
  }, [blob])

  useEffect(() => {
    return () => {
      URL.revokeObjectURL(url)
    }
  }, [url])

  return <img src={url} />
}

proposed behaviour:

export function createURL(blob) {
  return URL.createObjectURL(blob)
}

export function cleanupURL(url) {
  return () => {
    URL.revokeObjectURL(url)
  }
}

export function Image({ blob }) {
  const url = useMemo(createURL, [blob])
  // This particular case works here too
  // const url = useMemo(URL.createObjectURL, [blob])

  useEffect(cleanupURL, [url])

  return <img src={url} />
}

This means createURL and revokeURL are now testable standalone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants