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

[Task] Update useSubscriptions hook to reduce re-rendering #528

Closed
tthvo opened this issue Sep 30, 2022 · 3 comments · Fixed by #529
Closed

[Task] Update useSubscriptions hook to reduce re-rendering #528

tthvo opened this issue Sep 30, 2022 · 3 comments · Fixed by #529
Labels
chore Refactor, rename, cleanup, etc. question Further information is requested

Comments

@tthvo
Copy link
Member

tthvo commented Sep 30, 2022

Right now, we define useSubscriptions:

export function useSubscriptions() {
  const subsRef = React.useRef([] as Subscription[]);
  const [subscriptions, setSubscriptions] = React.useState(subsRef.current);

  React.useEffect(() => () => subsRef.current.forEach((s: Subscription): void => s.unsubscribe()), []);

  const addSubscription = (sub: Subscription): void => {
    setSubscriptions((subs: Subscription[]): Subscription[] => {
      const result = subs.concat([sub]);
      subsRef.current = result;
      return result;
    });
  };

  return React.useCallback(addSubscription, [setSubscriptions]);
}

which holds a local state of subcriptions. However, we already have the React.ref to store such subs. And, components does not need to re-render if the list of subscriptions update as its sub's result will update the component anyway.

My thought now is that we just need to use React.ref:

export function useSubscriptions() {
  const subsRef = React.useRef([] as Subscription[]);

  React.useEffect(() => () => subsRef.current.forEach((s: Subscription): void => s.unsubscribe()), []);

  const addSubscription = (sub: Subscription): void => {
    subsRef.current = subsRef.current.concat([sub]);
  };

  return React.useCallback(addSubscription, [subsRef]);
}
@tthvo tthvo added question Further information is requested chore Refactor, rename, cleanup, etc. labels Sep 30, 2022
@tthvo
Copy link
Member Author

tthvo commented Sep 30, 2022

What do you think @andrewazores?

@andrewazores
Copy link
Member

Looks like it makes sense. Could this be related to the performance issues Max observes in #526? The React 18 release notes talk about batched rendering changes for improved performance, but maybe the way we're tracking these subscriptions actually degrades performance with React's changes?

@andrewazores andrewazores moved this to Todo in 2.2.0 Release Sep 30, 2022
@andrewazores andrewazores moved this from Todo to Stretch Goals in 2.2.0 Release Sep 30, 2022
@tthvo
Copy link
Member Author

tthvo commented Sep 30, 2022

Right! It seemed to improve the experience of the UI when I updated this useSub. But I can't tell if its the root issue as the recording tables still appear quite slow (i.e. top corner checkbox appears checked then unchecked on first load -> its intermediate states are shown).

@tthvo tthvo changed the title Update useSubscriptions hook to reduce re-rendering [Task] Update useSubscriptions hook to reduce re-rendering Sep 30, 2022
Repository owner moved this from Stretch Goals to Done in 2.2.0 Release Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. question Further information is requested
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants