-
Notifications
You must be signed in to change notification settings - Fork 133
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
fix: Variant build stylesheet for RNW #121
Conversation
a478d04
to
e0caf45
Compare
Sorry to bother, but I wasn't able to tag Reviewers when creating this PR for some reason, so I will ping in comment. @drmas @JoelBesada is there a chance we can merge this PR, or maybe there is something more that should be addressed? |
@dobrea-v I can take a look! |
We also have this issue with text styles from variants and applying this PR fixes the bug for us! I wonder what the root cause of this bug is? I think there's a RNW bug hidden here |
Thank you @linddominic
Maybe there are some differences in implementation between RN and RWN regarding StyleSheet API, I will try to dig deeper. To be more clear about bug, in description I wrote 'incorrect styles generation for variants', but that not actually true, more precisely is that we are loosing variants styles. This happens because we run variant styles two times through 'buildStyle' function (which now returns StyleSheet.create, the return is a reference to StyleSheetRegistry), and we get something like StyleSheet.create(StyleSheet.create(props)), which it cannot resolve. |
@dobrea-v ahh, thank you for your thoughts! In RN, Stylesheet.create is actually a no-op: So my theory is that RNW compiles Stylesheet.create to a CSS class, it does not support nesting and that causes a deviance in behavior on RNW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I just tested this PR and it turns out that #94 is effectively undone by this PR. All styles are being applied inline using the style
prop instead of CSS classes... 😕
If that is the fix, why not undo #94 to keep the code simple? (not a rhetorical question, this is an actual suggestion)
I looked into react-native-web's Stylesheet API and I think calling Stylesheet.create
in the render loop for each component instance causes a ton of style IDs to be generated. This is a memory leak
Relevant code:
Thanks @Yonom for pointing to the fact that Stylesheet.create is no-op in RN, this confirms that the performance impact is negligible for RN. Regarding your reply:
Firstly this is not true, it can be seen even without testing, this PR cannot possibly un-done #94, just because the changes are just in createVariant file, which has no impact on all other parts, only variants. (if you don't have variants in components, it's not even run once). Secondly I double-checked my changes just in case, and this PR does generate atomic CSS classes as it should, for both style props and variants, and there is not a single style property. More then that, having multiple instances of same component, they share same atomic CSS (which should be obviously intended, but I decided to mention it just in case). Here is a screenshot from my storybook Just to proof that i'm not getting this out of the blue, here is another screenshot, where you can see the changes from this PR If you can show me your tests, so we can figure out what is wrong on your side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dobrea-v sorry, that was a sloppy test on my part, I had another patch-package applied to my code (which reverted #94 🤦)
I can confirm that this test works alright. This looks like an improvement over the current situation. 👍
However, the issue regarding the memory leak is still there (styles are stored in ReactNativePropRegistry and a new style ID created every render, making the object grow over time)
@Yonom that's ok, I'm glad that everything worked out.
When we will merge this PR, I will try to create some performance benchmarks between versions. I will need them anyway for our product, since we are planing to re-use our mobile app components (made with restyle) on web, and I don't want surprises there 😀. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@linddominic @drmas Is there a chance we can merge this PR, as you can see in this thread we made some tests and it solves the issue. Also as i can see many are waiting for this fix. Thanks in advance |
This fix should also be added to the |
I am also using the |
Hey, @RikSchefferOberon
Not sure what is the use case. As far as I know @edgaryp sure no problem, I am glad to help restyle because I really enjoy working with this lib. but I still I don't see any attention for this PR from maintainers, which is a bit disappointing. Maybe @ElviraBurchik can help. |
Thanks for your contribution, @dobrea-v 👏 |
Thanks @ElviraBurchik 🥳 |
@ElviraBurchik can we create a new npm version with this fix? In change log I would suggest to write: Fix web support for variant's Appreciate your help 🙏 |
@dobrea-v sure, I will do that tomorrow 👌 |
This PR tries to fix a bug on RNW, which consists in incorrect styles generation for variants. The issue is also mentioned here by @chris24elias
It happened after #94 fix, which introduced use of StyleSheet.create in buildStyle, however for createVariant this is causing incorrect styles resolution.
It can be seen in this codesanbox
Checked this fix on the web, and variants are working as expected, with atomic classes being generated. And on RN everything works as expected.
I understand that this library doesn't have official support for RNW, but would be nice to have variants working on the web.