bmcmahen/sancho

Switching colors should be done by props, but not components like <LightMode/> and <DarkMode/>

Opened this issue · 1 comments

I think the current way of switching dark/light modes described in https://sancho-ui.com/#theme has a drawback that it forces the child components to re-render
bacause the top-level component changes to the different one, e.g. from LightMode to DarkMode and vice versa.
This React's behavior is described in
https://reactjs.org/docs/reconciliation.html#elements-of-different-types

Whenever the root elements have different types, React will tear down the old tree and build the new tree from scratch.

In most cases, <LightMode /> and <DarkMode /> are the root or near to the root component and re-building its child components takes high cost.
In addition, forcing the children to re-render causes some undesirable/unexpected effects.

Please see the exapmle below.
In the root (App.tsx), the light/dark modes are toggled by changing the root <LightMode /> and <DarkMode /> components.
In the child component (DataFetcher.tsx), it fetches data from an API in async way with useEffect. In this example, the API is dummy and configured to take 1 second for example.

App.tsx

import React, { useState } from 'react';
import { LightMode, DarkMode, Button } from 'sancho';
import DataFetcher from './DataFetcher';

const App: React.FC = () => {
  const [darkMode, setDarkMode] = useState(false);

  const Mode = darkMode ? DarkMode : LightMode

  return (
    <Mode>
      <div>
        <DataFetcher />

        <Button onClick={() => setDarkMode(!darkMode)}>Toggle Dark Mode</Button>
      </div>
    </Mode>
  );
}

export default App;

DataFetcher.tsx

import React, { useState, useEffect } from 'react';

interface DummyData {
  name: string;
}

const dummyAPI = (): Promise<DummyData> => {
  const dummyData: DummyData = {
    name: 'foo'
  };

  return new Promise((resolve) => {
    setTimeout(() => resolve(dummyData), 1000);
  });
}

const DataFetcher = () => {
  const [data, setData] = useState<DummyData>();

  useEffect(() => {
    dummyAPI().then(setData);
  }, []);

  return (
    <div>
      {data && data.name}
    </div>
  );
}

export default DataFetcher;

When you run this code, you will see that each time you toggle the color mode, the fetching occurs and there are 1s delays for the fetched data to be shown:
sancho-rerender-example

This is because, as I wrote above, changing the root component (<LightMode /> and <DarkMode />) forces the child component to re-render and it triggers the fetching in useEffect.

To solve this, I suggest that you provide a single theming component and switching color mode is done by changing its props.
I think ColorMode component defined in https://github.com/bmcmahen/sancho/blob/master/src/Theme/Providers.tsx , which is currently not public is good fit for this purpose.

For example, if the App.tsx is changed like below, the problem is solved.

import React, { useState, useRef } from 'react';
import { useTheme, Theme, ThemeColors, ThemeProvider, Button } from 'sancho';
import DataFetcher from './DataFetcher';

const ColorMode = ...  // ColorMode and its deps are now not public and just copied from the source code.

/**
 * Main app
 */
const App: React.FC = () => {
  const [darkMode, setDarkMode] = useState(false);

  const theme = useTheme();
  const colors = darkMode ? theme.modes.dark : theme.modes.light;

  const colorModeRef = useRef();  // I think this ref should be optional when `ColorMode` is public API.

  return (
    <ColorMode colors={colors} ref={colorModeRef}>
      <div>
        <DataFetcher />

        <Button onClick={() => setDarkMode(!darkMode)}>Toggle Dark Mode</Button>
      </div>
    </ColorMode>
  );
}

export default App;

sancho-no-rerender-example
This shows that unnecessary re-rendering of the child does not occur.

This is a great post -- thanks for creating it.

I'm actually surprised that changing the color mode actually causes the component to remount, but it makes sense given your explanation. That's definitely not ideal, and really shouldn't be necessary. I'd like to maintain the current API if possible - so let me explore potential ways to do that, and if I can't figure anything else we can fallback to your solution.