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

Idea: rewrite useSet and useMap to avoid building a new instance on each update #1188

Open
mattboldt opened this issue May 4, 2020 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mattboldt
Copy link

The problem
I noticed these two hooks are effectively re-implementing the native Set and Map classes, and performing some potentially expensive operations on them. Ex:

https://github.com/streamich/react-use/blob/master/src/useSet.ts
https://github.com/streamich/react-use/blob/master/src/useMap.ts

react-use/src/useSet.ts

Lines 19 to 20 in 0572ced

const add = (item: K) => setSet(prevSet => new Set([...Array.from(prevSet), item]));
const remove = (item: K) => setSet(prevSet => new Set(Array.from(prevSet).filter(i => i !== item)));

Here we're creating a copy of the set by transforming it into an array via Array.from. Then, we're performing a filter or a spread operator on them to emulate what Set#add and Set#delete would be doing.

Solution
Inspired by mobx-react-lite and how they handle Set and Map, followed by forcing a re-render, I decided to try to build a custom hook for vanilla React.

Here's a proof of concept: https://codesandbox.io/s/react-set-map-hooks-proxy-v9wzw?file=/src/App.js

Essentially, instead of relying on useState to trigger updates, requiring us to copy and re-instantiate our Set, I'm using useRef to store the Set object. I also implemented Proxy to help keep the API the exact same so no custom functions are required.

I've yet to do performance tests, but plan to soon. I'd love to see this in use on large data sets so it can take advantage of Set's built-in optimizations.

@laleksiunas
Copy link

I would like to add that Proxy is not supported in older browsers like IE11. Also, why do we need to use Proxies here when we are already 'proxying' the backing set while returning add, remove, and other modifiers from the hook? We may add logic to these functions to force rerender when called. An ideal implementation would be to check if the set actually changed and needs rerender (yet this functionality should be configurable using a boolean flag like 'pure' to prevent adding a runtime penalty when it is not needed)

@streamich
Copy link
Owner

I like the idea of providing the exact same APIs as Set and Map and storing the state aside from useState and triggering updates manually.

But I'm skeptical about introducing Proxy here, due to performance penalty and unavailability in older browsers.

@streamich streamich added good first issue Good for newcomers help wanted Extra attention is needed enhancement New feature or request labels May 16, 2020
@mattboldt
Copy link
Author

Great points @laleksiunas @streamich! You could definitely have a pseudo "proxy" instead of Proxy and also only force render on mutations (i.e. based on the response from set.add() or map.set()). I'll try to update my demo soon and get a PR open.

@laleksiunas
Copy link

laleksiunas commented May 18, 2020

@mattboldt I was thinking about a builder function that can create this kind of hooks, like the one below (just straight out of my mind, not tested):

const createDataEntityHook = (createDefaultValue, createModifiers) => () => {
  const forceUpdate = useForceUpdate();

  const backingEntity = useMemo(createDefaultValue, []);
  const modifiers = useMemo(() => createModifiers(backingEntity, forceUpdate), []);

  return [backingEntity, modifiers];
};

And to use it:

export const useSet = createDataEntityHook(
  () => new Set(),
  (set, update) => ({
    add: value => {
      const setSizeBefore = set.size;

      set.add(value);

      // check if set changed to prevent unnecessary rerenders (might be turned off by configuration)
      if (setSizeBefore !== set.size) {
        update();
      }
    }
    // other functions
  })
);

What do you think?

@JoeDuncko
Copy link

Hi all! @react-hookz/web, the new library by one of react-use's former maintainers (background here and here) has purposely implemented its hooks to return the same reference object on every render, for every hook, including useSet and useMap.

For those interested, there's an official migration guide for migrating from react-use to @react-hookz/web.

Hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants