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

[v3] Weird conflicts between same style object name #547

Closed
jeremybarbet opened this issue Jan 31, 2025 · 12 comments · Fixed by #552
Closed

[v3] Weird conflicts between same style object name #547

jeremybarbet opened this issue Jan 31, 2025 · 12 comments · Fixed by #552

Comments

@jeremybarbet
Copy link
Contributor

jeremybarbet commented Jan 31, 2025

Description

Hi there,

I'm not a big fan of opening issues without reproducer, but couldn't create one and wanted to get your opinion, so I can debug it further.

When I create components, the first wrapping View (or any other component) with style will get the name wrapper

const MyComponent = () => (
  <View style={s.wrapper}>...</View>
)

const s = StyleSheet.create({
  wrapper: {}
})

So I have a Card.tsx component, really nothing incredible about it.

export const Card = ({ children, style }: CardProps) => <View style={[s.wrapper, style]}>{children}</View>;

const s = StyleSheet.create(theme => ({
  wrapper: {
    borderRadius: theme.spacings.md,
    borderCurve: 'continuous',
    borderWidth: 1,
    borderColor: theme.form.stroke,
    backgroundColor: theme.background,
  },
}));

It is NOT used or imported in the screen from the video, but somehow its styles gets injected when I switch from dark to light theme. I can confirm it's this component, because if I change its style it will show when switching.

Below 2 videos. First one when I have borderWidth: 1, second one when I tweaked to borderWidth: 10 just to confirm it's coming from this component.

border-width-1.mov
border-width-10.mov

So if I rename this component's style to wrapper2, for example, then the issue disappear.

Screen.Recording.2025-01-31.at.16.37.51.mov

But I have about 40 components that uses this wrapper first style object, so I wonder if I could get some other conflicts at some point somewhere else.

If you have any idea where it could come from, babel-plugin side, native side, what I could log that would help finding the issue. Let me know!

Steps to Reproduce

  1. Pull branch repro/animated-theme from feat: add repro for animated and theme issue jeremybarbet/unistyles-sandbox#3

Snack or Repository Link (Optional)

jeremybarbet/unistyles-sandbox#3

Unistyles Version

3.0.0-beta.6

React Native Version

0.76.6

Platforms

iOS

Expo

Yes

@jpudysz
Copy link
Owner

jpudysz commented Jan 31, 2025

No worries, we will track the issue.

It is NOT used or imported in the screen from the video

Can you show me component code from the video though?

From my POV:

  • you're switching the theme, so Unistyles emits event about it
  • on C++ side HybridStyleSheet::onPlatformNativeDependenciesChange method is called
  • this function calls getStyleSheetsToRefresh so any StyleSheet that depends on theme will be taken in count
  • finally I'm recomputing all styles in all StyleSheets that depends on theme
  • all styles are passed to the ShadowTree and your components are updated

@jeremybarbet
Copy link
Contributor Author

jeremybarbet commented Jan 31, 2025

Hum, okay found something interesting, probably the root cause. I was removing as much boilerplate as possible to get a reproducer.

So the text you are seeing is using this component:

export const Typography = ({ children, type = 'P1', color = 'primary', numberOfLines, style }: TypographyProps) => {
  return (
    <Animated.Text style={[s.wrapper, style]} numberOfLines={numberOfLines}>
      {children}
    </Animated.Text>
  );
};

quick video:

Screen.Recording.2025-01-31.at.17.12.01.mov

Now, if I remove Reanimated's Animated:

export const Typography = ({ children, type = 'P1', color = 'primary', numberOfLines, style }: TypographyProps) => {
  return (
-    <Animated.Text style={[s.wrapper, style]} numberOfLines={numberOfLines}>
+    <Text style={[s.wrapper, style]} numberOfLines={numberOfLines}>
      {children}
+    </Text>
-    </Animated.Text>
  );
};

it now works:

Screen.Recording.2025-01-31.at.17.13.27.mov

Next, the big border you see around the screen? I have a ScrollView, which is wrapped with Reanimated as well. If I update as follow:

- <Animated.ScrollView>...
+ <ScrollView>...

Then the issue disappear too:

Screen.Recording.2025-01-31.at.17.15.22.mov

And just to confirm, I put back all my components and styles together without the Animated. stuff and it works

Screen.Recording.2025-01-31.at.17.16.39.mov

I will try to get a reproducer up with these new findings

@jeremybarbet
Copy link
Contributor Author

Alright, here is a repro jeremybarbet/unistyles-sandbox#3. It doesn't produces the same visual glitch, but it should be good enough to track down the issue?

With Reanimated:

return <Animated.Text style={s.wrapper}>{children}</Animated.Text>;
Screen.Recording.2025-01-31.at.18.06.20.mov

Without reanimated:

return <Text style={s.wrapper}>{children}</Text>;
Screen.Recording.2025-01-31.at.18.06.33.mov

@jeremybarbet
Copy link
Contributor Author

If I comment out this line here

const REPLACE_WITH_UNISTYLES_PATHS = [
-    'react-native-reanimated/src/component',
    'react-native-gesture-handler/src/components'
]

It then works as it should be and I don't see any regression in my app elsewhere.

It does also fix this issue at the same time.

@jpudysz jpudysz added this to the 3.0.0-beta.7 milestone Feb 1, 2025
@jpudysz
Copy link
Owner

jpudysz commented Feb 1, 2025

There will be regression. Theme change won't affect Reanimated components. We need this line in Babel.

Thanks for all the details, I will debug it 🕵️‍♂️

@jeremybarbet
Copy link
Contributor Author

jeremybarbet commented Feb 1, 2025

Yeah that's what I thought. But because I'm using useUnistyles in almost all my components with Reanimated to use along interpolateColor I guess it forces a re-render anyway when the theme changes, so I don't see these regressions

Thanks!

Edit: Well, I do see some regressions in some places, as you said

@jpudysz
Copy link
Owner

jpudysz commented Feb 3, 2025

Damn, @jeremybarbet, I know why that’s happening 😬

As you know from #512, Reanimated flattens the style array. This also means I lose the C++ state.
Unistyles can restore it, but the current algorithm was based on styleKey (hash). Each wrapper had the same hash, so it only considered the first one. I had to debug it using pointer addresses and found that Animated.Text received the style from View.

Now, the hash is computed based on styleKey + stylesheetKey.

@jeremybarbet
Copy link
Contributor Author

Nice, I see, that make sense, thanks!

Would you mind publishing a nightly release?

@jpudysz
Copy link
Owner

jpudysz commented Feb 3, 2025

I'm working on one more task, will publish release later today.

@jpudysz
Copy link
Owner

jpudysz commented Feb 3, 2025

Released as nightly: 3.0.0-nightly-20250203

@jpudysz jpudysz added the nightly label Feb 3, 2025
@jeremybarbet
Copy link
Contributor Author

Works like a charm. Thank you so much for your hard work!

@jpudysz
Copy link
Owner

jpudysz commented Feb 3, 2025

You’re welcome! 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants