use-ink / contracts-ui

Web application for deploying wasm smart contracts on Substrate chains that include the FRAME contracts pallet
https://contracts-ui.substrate.io/
GNU General Public License v3.0
62 stars 48 forks source link

(discussion) The current React Context providers cause unnecessary rerenders #478

Closed NoriSte closed 1 year ago

NoriSte commented 1 year ago

I was asked to analyze the current application state management and optimization of re-renders. This issue is meant to be a discussion around the topic instead of a "let's do that now" solution. The reason why I want to discuss the issue before implementing it is because:

  1. I miss the context: I never contributed to the project before and I could completely miss the context of the real uses cases and problems.

  2. This is a static analysis: the issue I want to discuss does not come from analyzing complex use cases nor from some users reporting a problem.

Current situation

Almost all of the current Context Providers create a new instance of the object wrapping all the Context's data.

https://github.com/paritytech/contracts-ui/blob/master/src/ui/contexts/ApiContext.tsx#L75-L81

https://github.com/paritytech/contracts-ui/blob/master/src/ui/contexts/TransactionsContext.tsx#L120-L125

https://github.com/paritytech/contracts-ui/blob/master/src/ui/contexts/ThemeContext.tsx#L36

https://github.com/paritytech/contracts-ui/blob/master/src/ui/contexts/TransactionsContext.tsx#L124-L129

As a result, the reference to the object changes at every Context Provider rerender, causing all the children components consuming the Context values to rerender, even if they use a portion of the Context values.

Consider this use case

<ThemeContextProvider>
  <ApiContextProvider> // <-- 1. ApiContextProvider rerenders because of an internal state change
    <DatabaseContextProvider>
      <TransactionsContextProvider> // <-- 2. As a result TransactionsContextProvider rerenders and changes the reference of the Context's value even if the content did not change
        <div>
          <Sidebar />
          <AwaitApis>
            <Outlet /> // <-- 3. Down in the component tree, the consumers of TransactionsContextProvider's value rerenders (independently from memoization) because the value object is new
          </AwaitApis>
        </div>
      </TransactionsContextProvider>
    </DatabaseContextProvider>
  </ApiContextProvider>

Proposed solution

Wrapping all the above-mentioned objects with React.useMemo to reduce the unnecessary deep rerenders.

Devil's advocate

What if we don't need to optimize the current Context Providers?

  1. Context Providers are not rerendered often: by reading the code of the Context Providers, it seems to me that apart from some re-renders caused by important events (the API websocket connection, for instance), the Context Providers do not re-render often. I need help and more context here to understand if this assumption is true or not.

Conclusion

I'd like to discuss the problem before jumping on implementation, since gaining info about the context (and the future direction of the UI) is crucial to understand if fixing the reported problem should be done or not.

Notes

I did not use any of the existing templates because this issue is not a Bug report, nor a Feature request, nor a Security vulnerability.