-
-
Notifications
You must be signed in to change notification settings - Fork 149
Fix fontWeight again #806
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 fontWeight again #806
Conversation
87b0cc3
to
b24f019
Compare
b24f019
to
a134bc7
Compare
a134bc7
to
239d39a
Compare
@cknitt which version of Number values are accepted since I didn't experience any crash on Android or iOS. |
That's weird as I had the crash on Android with 0.73.5. |
I'm still on 0.71.14 so maybe there is a regression on this in |
I tested it on a 0.73.4 project and it work well on Android, without crashing. |
Sorry, patched this in the project I was working on at the time and then lost track of it. Now a colleague has encountered the same problem in another project with RN 0.74.2. So this is definitely still an issue. Will merge now. |
I still not experienced the issue since 0.71 Are you using a custom font? Are you using the new architecture? |
@cknitt Hi!
It's seems to be definitively an issue… but only on your project? Can you repro the issue outside? This type was change 1 year ago, no one else seems to experienced the issue since, and I'm worried the revert to the old type will annoy people… |
This is not specific to a particular project of ours. My colleague encountered the issue in a new project, not in the one where I originally experienced it. I will try to create a repo with a simple reproduction. |
@Freddy03h Ok, here is a simple repro:
The problem does not occur for the other styles in that file. |
@cknitt Hi! Thank you for all the informations! I opened an issue an the I never experienced the issue on my project because now with dynamic style (dark/light mode) there's always a variable color to dynamically pass to the style prop in an array. And because the error is not effective on Release mode, I never saw it on crashlytics (even with 2 text in my entire app where the issue should happen). It's possible to create a patch-package with this tiny fix that will be fixed soon I hope : https://github.com/facebook/react-native/pull/45299/files So, what's your opinion about reverting the type to the polymorphic variant? I think it would be nice not to have to rewrite the type for everyone for an issue that happen only in Dev mode in the old architecture. Thank you. |
Hi @Freddy03h! Thank you very much for opening the issue on the React Native repo to get to the bottom of this.
I can confirm that this fixes the issue. This solution is fine with me, I'll make a PR to revert the type to the polymorphic variant. |
Fixes #775 again by reverting the
fontWeight
type definition to its pre-#793 state.Unfortunately this is a breaking change yet again. But better than crashes on Android...