satya164 / react-native-tab-view

A cross-platform Tab View component for React Native
MIT License
5.14k stars 1.07k forks source link

Many useless re-renders #1396

Closed cglacet closed 1 year ago

cglacet commented 1 year ago

Current behavior

The following code will render each tab two times on init and one time on every tab change.

import * as React from 'react';
import { Dimensions, Text } from 'react-native';
import { TabView, SceneMap } from 'react-native-tab-view';

const renderCount = new Map();

export default function App(){
  return <Nav nbTabs={2} />;
}

function Nav({nbTabs}:{nbTabs: number}){
  const [index, setIndex] = React.useState(0);
  const routes = React.useMemo(() => routesParams(nbTabs), [nbTabs]);

  const renderScene = SceneMap(
        Object.fromEntries(
            routes.map((c) => [
                c.key,
                () => <Route {...c}/>
            ])
        )
    );

  return <TabView 
      navigationState={{ index, routes }}
      renderScene={renderScene}
      onIndexChange={setIndex}
      initialLayout={{ width: Dimensions.get('window').width }}
  />
}

interface RouteParams {
  key: number;
  id: number;
  title: string;
}

function Route({title, id}: RouteParams){
  const tabRenderCount = (renderCount.get(id) ?? 0) + 1;
  renderCount.set(id, tabRenderCount);
  console.log(`Re-rendering ${title} (${tabRenderCount} times in total)`);
  return <Text>{title}</Text>;
}

function routeParam(value: number): RouteParams{
  return {key: value, id: value, title: `Tab ${value}`}
}

function routesParams(quantity: number): RouteParams[] {
  return [...Array(quantity).keys()].map(i => routeParam(i));
}

We can add some logs to check the code on our side is doing what we excpect. For example we can add a log within the routes useMemo, and indeed it's only called once (during init) :

Route params changed.
Re-rendering Tab 0 (1 times in total)
Re-rendering Tab 1 (1 times in total)
# Init re-render
Re-rendering Tab 0 (2 times in total)
Re-rendering Tab 1 (2 times in total)
# clicked on tab 1 => re-renders again
Re-rendering Tab 0 (3 times in total)
Re-rendering Tab 1 (3 times in total)

The changed part of the code:

const routes = React.useMemo(() => {
    console.log("Route params changed.");
    return routesParams(nbTabs)
  }, [nbTabs]);

Even if the docs says SceneMap acts as a memoizer for routes, I also checked this wasn't the issue (encapsulating it in useMemo). This doesn't change anythin.

Expected behavior

All routes should render exactly once.

Reproduction

https://snack.expo.dev/jKHFsGJ94

Platform

Environment

The environement is the one linked in the documentation:

package version
react-native-tab-view ^3.2.1
react-native-pager-view ^6.0.1
expo 46.0.0
github-actions[bot] commented 1 year ago

Couldn't find version numbers for the following packages in the issue:

Can you update the issue to include version numbers for those packages? The version numbers must match the format 1.2.3.

The versions mentioned in the issue for the following packages differ from the latest versions on npm:

Can you verify that the issue still exists after upgrading to the latest versions of these packages?

github-actions[bot] commented 1 year ago

Couldn't find version numbers for the following packages in the issue:

Can you update the issue to include version numbers for those packages? The version numbers must match the format 1.2.3.

The versions mentioned in the issue for the following packages differ from the latest versions on npm:

Can you verify that the issue still exists after upgrading to the latest versions of these packages?

cglacet commented 1 year ago

I also tried replacing the renderScene by a simple function, but this leads to the same bug:

const renderScene = React.useCallback(({route}: {route: RouteParams}) => 
    <Route {...route}/>, []
  );
cglacet commented 1 year ago

The bug also appears with static routes, the following also triggers useless re-renders even though the code uses constants in the Nav component.

const renderScene = SceneMap({0: Route, 1: Route});
const routes = routesParams(2);

function Nav({nbTabs}:{nbTabs: number}){
  const [index, setIndex] = React.useState(0);
  return <TabView 
      navigationState={{ index, routes }}
      renderScene={renderScene}
      onIndexChange={setIndex}
  />
}
satya164 commented 1 year ago

https://github.com/satya164/react-native-tab-view#avoid-unnecessary-re-renders

satya164 commented 1 year ago

If there's a bug with SceneMap or React.memo/React.PureComponent please open a separate issue with repro. Creating components in render/useMemo/useCallback aren't for memoing a component.

cglacet commented 1 year ago

Yes there is a bug with SceneMap. Render functions passed into it are called multiple times. I provided a reproduction here.

satya164 commented 1 year ago

SceneMap doesn't accept render functions. It accepts React components which should be defined outside of other components.

cglacet commented 1 year ago

Then the documentation is out of date as it clearly states that SceneMap excpect an objetct with key to function maping.

const FirstRoute = () => (...);
const renderScene = SceneMap({
  first: FirstRoute,
});
satya164 commented 1 year ago

That's a component defined outside of a component, not a regular function. Documentation also explicitly says not to pass a inline function: https://github.com/satya164/react-native-tab-view#renderscene-required

cglacet commented 1 year ago

Changing the code to:

const renderScene = SceneMap(
    Object.fromEntries(
        routes.map((c) => [
            c.key,
            Route,
        ])
    )
);

Doesn't make any difference. The problem is that render functions are called even when the input props (route params for exemple) are untouched.

satya164 commented 1 year ago

please open a new issue with a minimal repro that uses only SceneMap/React.memo if it's not working properly

cglacet commented 1 year ago

Why a new one? This one shows exactly how it's not working properly.

I think I found the issue in the code, here there is something like:

return (
    <>
        {
            navigationState.routes.map((route, i) => {
                renderScene(route);
            })
        };
    </>
);

But it should be something like:

const scenes = useMemo(() => {
    navigationState.routes.map((route, i) => {
        renderScene(route);
    });
}, [navigationState.routes]);

return <>{scenes}</>;
satya164 commented 1 year ago

This one has incorrect snack and incorrect code in the description. It's better to create a new issue with proper code and repro so the person looking into it doesn't need to read the whole discussion.

cglacet commented 1 year ago

What do you mean "incorrect snack"? The snack shows an example where route props are not managed properly even when using SceneMap.

satya164 commented 1 year ago

But it should be something like:

Memoizing components doesn't work that way. This is also unsafe and will prevent re-renders even if renderScene refers to a variable in the closure. If renderScene is used inside useMemo then it should be in dependency array to be used safely.

What do you mean "incorrect snack"

As I said, you cannot create components inside other components which is what your snack does and the documentation explicitly says not to do. Please create a minimal repro following the docs if there is a bug.