Skip to content
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

RFC: Memoize style objects to prevent re-renders of memoized components #27

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

levibuzolic
Copy link
Contributor

@levibuzolic levibuzolic commented Oct 11, 2023

Awesome work Jacek on a great library, I love the simple approach and it fits well with patterns I've used across a few apps already and I'm looking forward to seeing how it evolves.

I'd like to request a feature/open a discussion about how to better handle styles in a way that doesn't break memoization of components (ie. React.memo())

Problem

While integrating react-native-unistyles into a project we've noticed that styles break memoization (React.memo) when any dynamic features are used (ie. anything requiring useStyles()). This is because the useStyles hook returns a new style object on every render, which causes the memoized component to re-render every time.

It does however work as expected when using purely static styles because no internal manipulation is done to the passed-in style object.

This can be demonstrated by this Expo Snack. Where every press of the Re-render button triggers both dynamic style components to re-render.

image

User-land solution

To work around this issue we've been creating a separate static style object alongside our dynamic styles, or avoided passing styles entirely. (As demonstrated in the above Expo Snack)

// Dynamic styles, all unsafe to pass through to a memoized component
const stylesheet = createStyleSheet(({colors}) => ({
  item: {
    backgroundColor: colors.fg.primary,
  }
  text: {
    fontSize: 12,
  },
}));

// Static styles, safe to use in memoized components
const staticStyles = createStyleSheet({
  other: {
    borderRadius: 4,
  }
});

This means we haven't been able to get the full benefits of Unistyles in the few heavily optimised areas of our codebase.

Proposed solution (contents of this PR)

A few targetted uses of useMemo throughout the useStyles hook means that we can make styles memoizable by default, and only recompute them when either the screen size or the theme changes.

This has a trade-off of potentially increased memory usage, though at the same time would significantly reduce the amount of object transforms (nested parseStyle calls) on every render.

Kapture 2023-10-12 at 00 23 04

@vercel
Copy link

vercel bot commented Oct 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-unistyles-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 13, 2023 8:39am

@jpudysz
Copy link
Owner

jpudysz commented Oct 11, 2023

Wow @levibuzolic , thank you a lot for this PR!

I need some time to test it, and I will get back to you (most likely tomorrow).

@jpudysz jpudysz added the enhancement New feature or request label Oct 11, 2023
Copy link
Owner

@jpudysz jpudysz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the initial tests, I must say that this is an excellent PR. Thank you for delving into it. It will greatly improve the library!

@@ -1,4 +1,4 @@
import { useContext } from 'react'
import { useContext, useMemo, useEffect, useRef } from 'react'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused useEffect and useRef

@@ -57,7 +58,7 @@ export const createUnistyles = <B extends Breakpoints, T = {}>(breakpoints: B) =
...acc,
[key]: parseStyle<ST, B>(style, breakpoint, screenSize, sortedBreakpointEntries)
})
}, {} as ST)
}, {} as ST), [breakpoint, screenSize, sortedBreakpointEntries])
Copy link
Owner

@jpudysz jpudysz Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to pass sortedBreakpointEntries to the deps. It's computed only once, before user uses useStyles. Value will never change.

Copy link
Contributor

@efstathiosntonas efstathiosntonas Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theme needs to be passed to the deps array too otherwise theme won't be changed after user changes it

click for demo video
Screen.Recording.2023-10-13.at.08.56.25.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, of course. It looks like parsedStyles is actually what should be passed in here, that's the actual dependency which relies on theme and stylesheet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah you're 💯 right! Thanks @levibuzolic

@levibuzolic
Copy link
Contributor Author

Thanks for catching those issues @jpudysz, should be sorted now. 👍

@jpudysz
Copy link
Owner

jpudysz commented Oct 13, 2023

I will merge it and fix it in a separate PR as your timezone is 9h ahead 😅 .
Thank you @levibuzolic for the PR

@jpudysz jpudysz merged commit ce5cd32 into jpudysz:main Oct 13, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants