tailwind-react-native-classnames icon indicating copy to clipboard operation
tailwind-react-native-classnames copied to clipboard

memoization issues with 3rd party libraries (incl. React Navigation)

Open chankruze opened this issue 3 years ago • 49 comments

Before posting this isuue, I already read the #97 and https://github.com/jaredh159/tailwind-react-native-classnames#taking-control-of-dark-mode thoroughly. From there I understand that, the theme change won't re-render components using memo.

My question is, how can we useAppColorScheme() with React Navigation and get this to work ?

I've created a minimal repo and also recorded the screen for the demo purpose.

Repo Link Video Demo

chankruze avatar Jan 15 '22 14:01 chankruze

Thanks for the time you took making the repo and the video.

I don't have an answer for you at this point. I've not used React Navigation with twrnc, so I've not encountered this issue personally. I just looked for a few minutes through the code of react navigation, and they do use React.useMemo pretty extensively, although it's not easy for me to see if they are memoizing core components, or other values.

I think the way to really figure this out would be to try determine for sure that React Navigation is memoizing a key point in your component heirarchy, and exactly where. I might try this by putting some console.log('rendering SomeComponent') in the render functions of some components until i can figure out where it stops rendering.

If you can isolate where the renders get blocked by use-memo, the next thing I would do is see if that component accepts some prop that I could change when the app color scheme changes, thus forcing it to re-render. You might even be able to pass the component some arbitrary key prop, forcing a rerender.

I'm busy working on other things right now, so I'm not going to do a really in-depth troubleshooting of your case at this point, but I would hope that you might be able to try the technique I described above, and possibly find a solution. If you do, it would be great if you could share it in this issue for others. If some sort of pattern emerges, then maybe we could incorporate it into the library itself.

If you get totally stuck and are not able to find a workaround, let me know, I might be able to set aside some time before too long to take a shot at it myself as well.

jaredh159 avatar Jan 17 '22 14:01 jaredh159

@jaredh159 Sure! I got all your points. I'll try the key prop way, cause this seems cleaner to me and I've used this way in a game I developed. Thank you for your replay by the way.

Meanwhile, I will use shopify restyle for theming purpose. But I believe this is a good issue. I'll try my best to help you with this.

Thanks ;)

chankruze avatar Jan 17 '22 14:01 chankruze

@chankruze Hi, how did you resolve this issue at the end ?

Jackmekiss avatar Jun 03 '22 18:06 Jackmekiss

@chankruze Hi, how did you resolve this issue at the end ?

I am using shopify restyle for theming purpose for now. It also helps in consistent ui.

chankruze avatar Jun 04 '22 05:06 chankruze

I've noticed a similar issue with React Navigation when using withDeviceColorScheme: true on iOS.

When the device theme changes between dark/light, navigating to a new page in the app successfully reflects the new theme except when navigating to the first screen in the stack (i.e. the "home page"). I believe it may be because I only navigate to that screen using React Navigation's goBack, which does not re-render the screen unless there has been a state change.

A workaround I'm using is to force the home page screen to always re-render with React Navigation's useFocusEffect and a changing key prop on the component, although I could see this being detrimental if the component is expensive to render. Anyone have any suggestions on better way? I tweaked the repo provided by @chankruze to use the device theme and with my solution to demonstrate:

https://github.com/tmlamb/rn-twrnc-exp/commit/c14280d6f01a89b0b8bcb77483eceb47dcd897bb

tmlamb avatar Jul 29 '22 16:07 tmlamb

The problem is present with all the memoized components, having a hook/HOC for all these components is not a technically good solution, I will try to find a solution for my application and make a PR if ever.

Update: Adding a key props on the React Navigation Navigator component with the current breakpoint as value works perfectly.

import { config } from '@ui/tailwind';

function useBreakpoint() {
  const { width } = useDeviceDimensions();

  const breakpoints = Object.keys(config.theme.screens).reverse();
  const breakpointIndex = Object.values(config.theme.screens)
    .map((breakpoint) => parseInt(breakpoint.replace('px', ''), 10))
    .sort()
    .reverse()
    .findIndex((breakpoint) => width >= breakpoint);

  return breakpoints[breakpointIndex];
}

export function GuestStack() {
  const breakpoint = useBreakpoint();

  return (
      <Navigator
        key={breakpoint}
        initialRouteName={...}
      >
         {/* Screens... */}
      </Navigator>
  );
}

JamesHemery avatar Aug 04 '22 15:08 JamesHemery

I'd like to share an improved approach I've found to handling system theme changes when using this library with React Navigation. The solution I described in my last comment was to naively re-render to root screen of the app whenever the user navigated to it via the useFocusEffect:

import React from 'react'
import { useFocusEffect } from '@react-navigation/native'
import { View } from "react-native";

export default function HomeScreen() {
  const [forceRenderKey, setForceRenderKey] = React.useState(0)
  useFocusEffect(
    React.useCallback(() => {
      setForceRenderKey(v => v + 1)
    }, [])
  )

  return (
    <View key={forceRenderKey}>
        // Home screen content...
    </View>
  )
}

This worked fine with a home/root screen that was fairly static and didn't rely on any async behavior to render. In a new app I'm working on, the home screen requires a network request to render content, so the above approach results in the network request being sent whenever the user navigates away then back to the home screen, and the visual delay that entails (not ideal at all). The improvement I found is to have the home screen keep a reference to the system theme, and compare to the current theme onFocusEffect and when the app switches from background to foreground, so that the key that forces rerender only increments when needed:

import React from 'react'
import { useFocusEffect } from '@react-navigation/native'
import { Appearance, AppState, View } from "react-native";


export default function HomeScreen() {
  const [forceRenderKey, setForceRenderKey] = React.useState(0)
  const colorScheme = React.useRef(Appearance.getColorScheme());

  const handleColorSchemeChange = () => {
    const systemColorScheme = Appearance.getColorScheme();
    if (colorScheme.current !== systemColorScheme) {
      setForceRenderKey((v: number) => v + 1);
      colorScheme.current = systemColorScheme;
    }
  };

  useFocusEffect(
    React.useCallback(handleColorSchemeChange, [])
  )

  const appState = React.useRef(AppState.currentState);
  useEffect(() => {
    const listener = AppState.addEventListener("change", (nextAppState) => {
      if (
        appState.current.match(/inactive|background/) &&
        nextAppState === "active"
      ) {
        handleColorSchemeChange();
      }
      appState.current = nextAppState;
    });
    return () => {
      listener.remove();
    };
  }, []);

  return (
    <View key={forceRenderKey}>
        // Home screen content...
    </View>
  )
}

There may be more to consider if you give users the ability to toggle your app's theme separately from the system theme, but I think that could be handled with a little additional state/logic.

It's also worth considering moving the foreground event listener to a HOC that can be reused on every screen in an app, since switching from background to foreground seems to cause visual issues in the React Navigation header even on pages that are not at the root of the React Navigation stack.

tmlamb avatar Oct 25 '22 22:10 tmlamb

@tmlamb that's really helpful, thanks for taking the time to make the detailed writeup with code samples, i appreciate it.

jaredh159 avatar Oct 26 '22 15:10 jaredh159

I found a much easier solution, but I am not sure if it's case-specific. Simply add a useColorScheme() hook call to the component that does not update w.r.t device's color scheme. This will trigger a rerender on the component every time the system color scheme changes.

Example:

export const RootStackNavigation = () => {
  return (
    <Stack.Navigator>
      <Stack.Screen name="HomeScreen" component={HomeScreen} />
    </Stack.Navigator>
  );
};

export const HomeScreen = () => {
  useColorScheme(); // <-- add this

  return (
    <View style={tw`bg-white dark:bg-black`} />
  );
}

Nova41 avatar Nov 12 '22 18:11 Nova41

I was able to solve this using the docs: Enabling Device-Context Prefixes

jeromecovington avatar Dec 02 '22 16:12 jeromecovington

I found a much easier solution, but I am not sure if it's case-specific. Simply add a useColorScheme() hook call to the component that does not update w.r.t device's color scheme. This will trigger a rerender on the component every time the system color scheme changes.

@Nova41 With this approach are you able to see a React Navigation Header component with a theme-dependent background also switch to the new color at the same time as the screen View? I tried in my own app and I'm having trouble getting the header to re-render along with the screen view.

tmlamb avatar Dec 04 '22 16:12 tmlamb

I found a much easier solution, but I am not sure if it's case-specific. Simply add a useColorScheme() hook call to the component that does not update w.r.t device's color scheme. This will trigger a rerender on the component every time the system color scheme changes.

@Nova41 With this approach are you able to see a React Navigation Header component with a theme-dependent background also switch to the new color at the same time as the screen View? I tried in my own app and I'm having trouble getting the header to re-render along with the screen view.

I am using a custom header rendered within the component so it wasn't really an issue for me. Regarding your case, I think the header from the navigation library would not be rerendered, as it is not handled by the component and thus the state update of the component triggered by the hook does not trigger a rerender on the header. Perhaps you can try manually triggering a rerender of the header from the component by using the ref to the navigation (useNavigation) to set the header title to the same one?

Nova41 avatar Dec 05 '22 17:12 Nova41

Update : our first solution cause reset on navigation state when device context (eg. dark mode) change.

To solve this, we first tried to create a styled HOC (like NativeWind) to create derivative components that watch device context and trigger a re-render on changes (eq. const StyledView = styled(View);).

However, with twrnc the style is compiled in the parent component (e.g. screen) when the component is called, so this didn't solve our problem. With expo-router, the problem is even greater, since it's not possible to trigger a re-render of the screens easily, even though the problem would be present with any memoised component.

Finally, we've solved the problem by stopping import the tw variable directly, and using a useTw hook instead.

import { useColorScheme, useWindowDimensions, Appearance } from 'react-native';
import { tw } from './tailwind';

export function useTw(){
  const window = useWindowDimensions();
  const colorScheme = useColorScheme();

  return tw;
}

In this way, every component that uses tw is re-rendered when the context changes. We looking for better solution.

@jaredh159 are you open to review a v4 RFC ?

JamesHemery avatar Aug 16 '23 15:08 JamesHemery

i definitely feel like this whole memoization thing needs to be solved in the library, the more i think about it. i'm not 100% convinced though that this is the right approach.

internally, tw has a function called deriveCacheGroup which basically is a string made up of parts that could invalidate the current cache based on prefixes changing in the device context, strings are basically like w900--h600--fs1.3--dark--retina. i'm wondering if there's a way we could expose a hook to break memoization when this changes, and then components could opt into adding a special prop that takes this memo-breaker. i'm wondering if that would be better than having to call useTw() every time you want to use the tw func. but i could be missing something major.

would you perhaps be willing to setup a small expo sample project that demostrantes the core problem that i could test on?

jaredh159 avatar Aug 17 '23 13:08 jaredh159

The problem isn't the twnrc cache, but the fact that all components using tw must be re-rendered when the context changes. This is not the case for memoized components or expo-router screens.

I'll setup a repo with all of these issues.

JamesHemery avatar Aug 17 '23 15:08 JamesHemery

yeah, i didn't think the cache was the problem, more that a change to the cache group key could serve as the exactly correct proxy for when to break memoization. isn't this issue at it's core a memoization issue as well?

jaredh159 avatar Aug 18 '23 13:08 jaredh159

yeah, i didn't think the cache was the problem, more that a change to the cache group key could serve as the exactly correct proxy for when to break memoization. isn't this issue at it's core a memoization issue as well?

The problem is more twrnc's ability to trigger re-renders on memoized React components. useDeviceContext can trigger a re-render when the context changes, but this has no impact on child components that are memoized :/

JamesHemery avatar Aug 28 '23 12:08 JamesHemery

Thanks for the great work on twrnc @jaredh159 . I am working on my first mobile project as a react web developer and this has been immensely helpful.

I hope we can get a permanent fix to this dark mode issue with react navigation soon.

reedwane avatar Nov 01 '23 08:11 reedwane

@reedwane I tried to work on this issue this morning, but I can't get the reproduction app that @chankruze so kindly made to build. Seems like it was built with a really old version of RN (0.64), and I got metro runtime errors. I tried updating RN and got other errors. I'm willing to try to address the trwrnc integration stuff, but I don't really want to fight through all the RN/expo issues just to be able to start working.

would you, or @chankruze or maybe @JamesHemery be willing to take a crack at updating the reproduction repo so that it works with updated dependencies, and builds on node 18 (or tell me which node version it will work on). if i can get the sample app working, I'll try to dig in and see if I can expose some hook that would potentially solve this. thanks!

jaredh159 avatar Nov 03 '23 12:11 jaredh159

@jaredh159 Sure will check again tonight and let you know. Happy to help ;)

chankruze avatar Nov 03 '23 12:11 chankruze

@chankruze thanks, maybe i spoke to soon, i created a new expo app and then added a few deps and copied your components over, and i have it building now. i'll submit a PR to your repo in a sec...

jaredh159 avatar Nov 03 '23 13:11 jaredh159

ok, so maybe this is a totally naive attempt, but i took a crack at this, and have @chankruze's repro app fixed (i think).

the basic idea is what i proposed earlier in this thread, and some people have experimented with, using the key prop to selectively force a re-render.

i made an experimental beta release of twrnc that causes useDeviceContext() to return a string that will be stable until something with the device context changes, hopefully suitable to pass as a key prop. here's the commit with the key changes:

https://github.com/jaredh159/tailwind-react-native-classnames/commit/9807b45829dd90b8365b7241c49577d55b0e877a

Then, I applied this memo buster as a key on the react navigation container. And when I did that, i observed the issue someone noted, where the react navigation container seems to lose it's place in the stack (in the sample app, the home page would re-render, even if i was on the "about" page).

so, i just moved the memo-busting onto the <Stack.Navigator /> component, and it seemed to work... again, i have no idea if this is a robust solution, so I'm wondering if any of you in this thread could try to find issues with it. here's the code where i integrated the memoization busting:

https://github.com/jaredh159/rn-twrnc-exp/commit/039afb87bcdaa5a44f73b50627fc3e773a77ae5e

if anyone wants to play around with it, you can clone my repo here https://github.com/jaredh159/rn-twrnc-exp and checkout the test-memo-buster branch

jaredh159 avatar Nov 03 '23 13:11 jaredh159

I understand what you did. Let me check this tonight and share the result.

chankruze avatar Nov 03 '23 13:11 chankruze

so, i just moved the memo-busting onto the <Stack.Navigator /> component, and it seemed to work... again, i have no idea if this is a robust solution, so I'm wondering if any of you in this thread could try to find issues with it.

After many research, move it on <Stack.Navigator /> will cause the same behavior if you have nested browsers. All my research led me to the opening of the RFC, which solves all the problems I encountered on a large application.

Currently this is how I use twrnc :

import { createContext, useContext, useEffect, useMemo, useRef } from 'react';
import { create, useDeviceContext } from 'twrnc';
import { useWindowDimensions } from 'react-native';
import type { ReactNode } from 'react';
import type { TwConfig, TailwindFn } from 'twrnc';

const TailwindContext = createContext<{
  instance: TailwindFn | null;
  key: string | null;
}>({
  instance: null,
  key: null,
});

export function TailwindProvider({ config, children }: { config: TwConfig; children: ReactNode }) {
  const [_, forceUpdate] = useReducer((x) => x + 1, 0);

  const dimensions = useWindowDimensions();

  const instance = useRef<TailwindFn>(create(config));
  const lastConfig = useRef<TwConfig>(config);

  useEffect(() => {
    if (lastConfig.current !== config) {
      instance.current = create(config);
      lastConfig.current = config;
      forceUpdate();
    }
  }, [config, forceUpdate]);

  const key = JSON.stringify(dimensions); // On my side key also contains color scheme and others informations about the styling context

  useDeviceContext(instance.current);

  const value = useMemo(
    () => ({
      instance: instance.current,
      key,
    }),
    [key]
  );

  return <TailwindContext.Provider value={value}>{children}</TailwindContext.Provider>;
}

export function useTw() {
  const tw = useContext(TailwindContext);
  if (!tw.instance) {
    throw new Error('Tailwind config not provided.');
  }

  return tw.instance;
}

By using custom useTw components are re-rendered when styling context change because our custom React context change. With this little trick you can also switch the tailwind config based on criteria such as color scheme.

JamesHemery avatar Nov 03 '23 14:11 JamesHemery

@JamesHemery yeah, i figured you would be able see a problem with this, thanks for chiming in. can you clarify what you mean by "nested browsers"? or is there any way that you could modify the reproduction repo to illustrate this problem. you very well might be right that something like useTw() is the right approach, but i'd like to see the problems hands-on before committing to that route. thanks!

jaredh159 avatar Nov 03 '23 17:11 jaredh159

@jaredh159 I think by "nested browsers" he meant nested navigators I guess.

chankruze avatar Nov 03 '23 17:11 chankruze

@jaredh159 I think by "nested browsers" he meant nested navigators I guess.

Yes my bad, nested navigators like a Tab Navigator inside a Stack Navigator.

JamesHemery avatar Nov 04 '23 14:11 JamesHemery

Thanks very much @jaredh159 . I have tested the beta release with my app and it works for the toggle. By the way, I created the project using expo-router template, and I am using a context provider to pass the values across the app. The only issue on this is that the components using tw do not render as the dark mode when the app first loads even though the color scheme is picked as dark. But when I toggle the view subsequently, it seems to work fine.

const AppContextProvider = ({ children }: { children: React.ReactNode }) => {
  const buster = useDeviceContext(tw, { withDeviceColorScheme: false });
  const [colorScheme, toggleColorScheme, setColorScheme] =
    useAppColorScheme(tw);

  const contexts = { colorScheme, toggleColorScheme, setColorScheme, buster };

  return <AppContext.Provider value={contexts}>{children}</AppContext.Provider>;
};

_layout.tsx:

function RootLayoutNav() {
  const { colorScheme, buster } = useAppContext();

  console.log(colorScheme, buster);

  return (
    <ThemeProvider value={colorScheme === "dark" ? DarkTheme : DefaultTheme}>
      <Stack key={buster}>
        <Stack.Screen name="(tabs)" options={{ headerShown: false }} />
        <Stack.Screen name="modal" options={{ presentation: "modal" }} />
      </Stack>
    </ThemeProvider>
  );
}

reedwane avatar Nov 04 '23 14:11 reedwane

but if I remove { withDeviceColorScheme: false } from the useDeviceContext, it loads up appropriately with the dark mode applied first time, but doesn't toggle.

reedwane avatar Nov 04 '23 15:11 reedwane

@reedwane - for your initialization issue, the useAppColorScheme() function accepts an optional second parameter, which allows you to set the initial value. could that be the thing you're missing?

@JamesHemery would it be simple to modify the reproduction app to demonstrate the issue with nested navigation contexts?

jaredh159 avatar Nov 06 '23 14:11 jaredh159