Skip to content

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

Merged
merged 1 commit into from
Jul 1, 2024
Merged

Fix fontWeight again #806

merged 1 commit into from
Jul 1, 2024

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Mar 4, 2024

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...

@cknitt cknitt force-pushed the fix-font-weight-again branch from 87b0cc3 to b24f019 Compare March 4, 2024 11:26
fhammerschmidt
fhammerschmidt previously approved these changes Mar 4, 2024
@cknitt cknitt force-pushed the fix-font-weight-again branch from a134bc7 to 239d39a Compare March 4, 2024 12:26
@Freddy03h
Copy link
Member

Freddy03h commented Mar 7, 2024

@cknitt which version of react-native are you using?

Number values are accepted since 0.71 : facebook/react-native#34598

I didn't experience any crash on Android or iOS.

@cknitt
Copy link
Member Author

cknitt commented Mar 7, 2024

That's weird as I had the crash on Android with 0.73.5.

@Freddy03h
Copy link
Member

I'm still on 0.71.14 so maybe there is a regression on this in react-native@0.73 but it's seems that those modifications and the tests still exists on main branch… so I don't know ^^'

@Freddy03h
Copy link
Member

I tested it on a 0.73.4 project and it work well on Android, without crashing.

@cknitt
Copy link
Member Author

cknitt commented Jul 1, 2024

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.

@cknitt cknitt merged commit 3629eb9 into main Jul 1, 2024
@cknitt cknitt deleted the fix-font-weight-again branch July 1, 2024 11:42
@Freddy03h
Copy link
Member

I still not experienced the issue since 0.71

Are you using a custom font?

Are you using the new architecture?

@Freddy03h
Copy link
Member

@cknitt Hi!

  • I tried using number on a 0.74 project and it doesn't crash on android.
  • I also never experienced a crash linked to this issue on my project with 25k users/day and a variety of android versions and configuration.
  • Tests with numbers existing on the react-native project.
  • I don't find any related issues on the RN project.

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…

@cknitt
Copy link
Member Author

cknitt commented Jul 3, 2024

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.

@cknitt
Copy link
Member Author

cknitt commented Jul 3, 2024

@Freddy03h Ok, here is a simple repro:

  1. Create a new RN project with npx react-native init.
  2. Run the Android project.
  3. At the bottom of App.tsx, in the highlight style, change fontWeight from '700' to 700.

The problem does not occur for the other styles in that file.
Maybe it is because the highlight style is applied on a nested <Text>.

Screenshot_1720004134

@Freddy03h
Copy link
Member

@cknitt Hi! Thank you for all the informations! I opened an issue an the react-native repo facebook/react-native#45285 and the error only happen in Dev mode with the use of Stylesheet.create and without the use of an array in the style prop.

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.

@cknitt
Copy link
Member Author

cknitt commented Jul 6, 2024

Hi @Freddy03h! Thank you very much for opening the issue on the React Native repo to get to the bottom of this.

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

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.

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

Successfully merging this pull request may close these issues.

fontWeight broken in master
3 participants