-
Notifications
You must be signed in to change notification settings - Fork 84
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
memoization issues with 3rd party libraries (incl. React Navigation) #112
Comments
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 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 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 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 Sure! 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 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. |
I've noticed a similar issue with React Navigation when using 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 A workaround I'm using is to force the home page screen to always re-render with React Navigation's |
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>
);
} |
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 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 that's really helpful, thanks for taking the time to make the detailed writeup with code samples, i appreciate it. |
I found a much easier solution, but I am not sure if it's case-specific. Simply add a 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`} />
);
} |
I was able to solve this using the docs: Enabling Device-Context Prefixes |
@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? |
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. 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 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 @jaredh159 are you open to review a v4 RFC ? |
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 would you perhaps be willing to setup a small expo sample project that demostrantes the core problem that i could test on? |
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. |
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 :/ |
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 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 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 Sure will check again tonight and let you know. Happy to help ;) |
@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... |
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 i made an experimental beta release of 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 jaredh159/rn-twrnc-exp@039afb8 if anyone wants to play around with it, you can clone my repo here https://github.com/jaredh159/rn-twrnc-exp and checkout the |
I understand what you did. Let me check this tonight and share the result. |
After many research, move it on 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 |
@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 |
@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. |
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 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>;
};
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>
);
} |
but if I remove |
@reedwane - for your initialization issue, the @JamesHemery would it be simple to modify the reproduction app to demonstrate the issue with nested navigation contexts? |
I have been thinking about that actually . I just haven't been able to go back to it. I'll try soon and let you know. Thanks a bunch |
The problem is much broader than that, and can be illustrated quite easily: all you need is a memoised component. Let's imagine I have this structure :
In this case, the Stack will change when twrnc context change (#112 (comment)) so screen 1 will also be re-rendered and the navigation state lost. Losing the navigation state is a problem, but the real problem is that since the Form component is memoised, there's no incentive to re-render. Reproduction (without router because it's more easy to read) : import {Text, TouchableOpacity, View} from 'react-native';
import {memo, useReducer} from "react";
import {create, useAppColorScheme, useDeviceContext} from "twrnc";
const tw = create({});
const ClassicComponent = () => {
return (
<View style={tw`m-5 mt-30 border`}>
<Text style={tw`text-lg`}>Classic component</Text>
<View style={tw`bg-gray-50 dark:bg-gray-900`}>
<Text style={tw`text-gray-900 dark:text-gray-50`}>My text in classic component will be updated</Text>
</View>
</View>
);
}
const MemoComponent = memo(() => {
const [_, forceRender] = useReducer((i) => i + 1, 0);
return (
<View style={tw`m-5 border`}>
<Text style={tw`text-lg`}>Memoised component</Text>
<View style={tw`bg-gray-50 dark:bg-gray-900`}>
<Text style={tw`text-gray-900 dark:text-gray-50`}>My text in memoised component will not be updated</Text>
</View>
<TouchableOpacity style={tw`mt-4 p-3 bg-green-500`} onPress={forceRender}>
<Text>Force render memoised component</Text>
</TouchableOpacity>
</View>
);
});
export default function Screen() {
useDeviceContext(tw, {withDeviceColorScheme: false});
const [colorSchema, toggle] = useAppColorScheme(tw);
return (
<View>
<ClassicComponent/>
<MemoComponent/>
<TouchableOpacity style={tw`mt-4 mx-5 p-3 bg-green-500`} onPress={toggle}>
<Text>Toggle scheme</Text>
</TouchableOpacity>
</View>
);
} |
I set the second parameter using the value from But the first render still returns a light mode, then toggling switches to light mode and then properly to dark mode. Logging the colorScheme without using the second parameter on |
Do we have any valid fix for this issue yet? |
Thanks for this solution.However, This doesn't work for me. below is how I'm using the code
|
@JamesHemery thanks for the explanation and reproduction. sorry i reply so infrequently, as i've said before, i'm not facing this problem myself, or actively consuming this library (my rn apps are in maintenance mode), so it's hard to find time and motivation to dig into this. i understand what you're saying about any memoized component being a problem, but in my mind, it seems like the solution could be just to bust the memoization of any memoized component, using the memo buster key approach. like in you're example, i can fix it by just changing one line to: <MemoComponent key={memoBuster} /> you might say, that's onerous for the developer to have to break memoization everywhere, and i sort of agree. however, i also think it's onerous to have to consume twrnc with a hook instead of a bare importable function. i like lots of my simple component to have implicit returns, and if we said that going forward you have to consume so, we could have both apis, where you can use the bare import wherever you're not having memo troubles, and then opt into does that make sense? do you have any thoughts on that, or can you show me where my thinking is wrong? once again, really appreciate all of the thought and input you've put into this thread and the RFC, etc. cc @Aycom366 |
@jaredh159 Sometimes it's not possible to force the re-render with a key (expo-router for example). Even if we manage to do this, it will reset navigation, for example, which is a pity for the end user. But I understand your point of view. |
I've managed to solve the issue by using the |
This has been the cause of my problem, using |
Now that React Compiler is out, we're heading towards the world where memoized components are the default, not the opt-in on a case-by-case basis. So I believe this problem will become much widespread than it currently is. I believe An alternative approach I'm proposing is inspired by react-strict-dom's Something like this: export const tw = {
View: function View(props) {
const tw = useTw()
return <View {...props} style={tw.style(props.style)} />
},
Text: // ...
// Do this for most common components like View, Text, Pressable, ScrollView, FlatList, etc.
}
// Usage
function App() {
return <tw.View style={'bg-blue-100''}>
<tw.Text style={['text-red-500', 'text-lg']}>Hello</tw.Text>
</View>
} For custom components or third-party components developers can simply use |
I think you can use a wrapper to wrap any component (just like |
@NiuGuohui const Tw_FlashList = props => {
const tw = useTw()
return <FlashList
{...props}
style={tw(props.style)}
contentContainerStyle={tw(props.contentContainerStyle)}
/>
} With // Maybe something like this, but much more complicated to implement correctly
const Tw_FlashList = styled(FlashList, {
style: true,
contentContainerStyle: true,
}) |
The other problem with The "true" fix is to use a babel plugin and statically analyse the use of the It's a lot more work -- but akin to what you'd really want for exact performance. |
same here, any solution ? or we still have to use |
A bit noob here, but how do I use this memobuster stuff? Doing like this is not working for me : import { Stack } from 'expo-router';
import tw, { useDeviceContext, useAppColorScheme } from 'twrnc';
export const unstable_settings = {
// Ensure that reloading on `/modal` keeps a back button present.
initialRouteName: '(tabs)',
};
export default function RootLayout() {
useDeviceContext(tw, {
observeDeviceColorSchemeChanges: false,
initialColorScheme: `device`,
});
const [colorScheme, toggleColorScheme, setColorScheme] = useAppColorScheme(tw);
return (
<Stack key={tw.memoBuster}>
<Stack.Screen name="(tabs)" options={{ headerShown: false }} />
</Stack>
);
}
``` |
@tomihbk that looks roughly correct. you could try putting it on the |
Dear @jaredh159 , thanks for your input, I was able to make it to work by putting the key on : Do you have any time frame where this issue will be gracefully solved ? |
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()
withReact 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
The text was updated successfully, but these errors were encountered: