[Android] fontWeight number value error if not in an array

Freddy03h commented 1 week ago

Description

fontWeight accept number values since 0.71

But there is an issue on Android on a specific case:

It's only happen on the old Architecture (no issue with new arch) and with a style created with Stylesheet.create and given to style prop directly, not in an array.

style from Stylesheet.create directly passed to style prop

// Error

const styles = StyleSheet.create({
  highlight: {
    fontWeight: 700,
  },
});

<Text style={styles.highlight}>App.tsx</Text>

style from Stylesheet.create given to style prop in an array

// No error

const styles = StyleSheet.create({
  highlight: {
    fontWeight: 700,
  },
});

<Text style={[styles.highlight]}>App.tsx</Text>

inline style

// No error

<Text style={{ fontWeight: 700 }}>App.tsx</Text>

style not created with Stylesheet.create

// No error

const styles = {
  highlight: {
    fontWeight: 700,
  },
};

<Text style={styles.highlight}>App.tsx</Text>

Steps to reproduce

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

React Native Version

0.74.3

Affected Platforms

Runtime - Android

Output of `npx react-native info`

System:
  OS: macOS 14.5
  CPU: (10) x64 Apple M1 Pro
  Memory: 24.67 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.16.0
    path: ~/.nvm/versions/node/v18.16.0/bin/node
  Yarn:
    version: 3.6.4
    path: /usr/local/bin/yarn
  npm:
    version: 9.5.1
    path: ~/.nvm/versions/node/v18.16.0/bin/npm
  Watchman:
    version: 2023.08.14.00
    path: /usr/local/bin/watchman
Managers:
  CocoaPods:
    version: 1.11.2
    path: /Users/freddy/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK:
    API Levels:
      - "27"
      - "28"
      - "29"
      - "30"
      - "31"
      - "33"
      - "34"
    Build Tools:
      - 27.0.3
      - 28.0.3
      - 29.0.3
      - 30.0.2
      - 30.0.3
      - 31.0.0
      - 32.0.0
      - 33.0.0
      - 34.0.0
    System Images:
      - android-27 | Google APIs Intel x86 Atom
      - android-29 | Google APIs Intel x86 Atom
      - android-30 | Google APIs ARM 64 v8a
      - android-31 | Google APIs ARM 64 v8a
      - android-33 | Google APIs ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2021.1 AI-211.7628.21.2111.8309675
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.11
    path: /usr/bin/javac
  Ruby:
    version: 2.7.4
    path: /Users/freddy/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.74.3
    wanted: 0.74.3
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: Not found
  newArchEnabled: false

Stacktrace or Logs

Error while updating property 'fontWeight in shadow node of type: RCTVirtualText
java.lang.Double cannot be cast to java.lang.String

updateShadowNodeProp
ViewManagersPropertyCache.java:125

setProperty
ViewManagerPropertyUpdater.java: 161

updateProps
ViewManagerPropertyUpdater.java: 65

updateProperties
ReactShadowNodeImp1. java: 321

updateView
UI Implementation.java: 279

updateView
UIManagerModule. java:433

invoke
Method. java

invoke
JavaMethodwrapper. java: 372

invoke
JavaModuleWrapper. java: 146

run
NativeRunnable. java

handleCallback
Handler.java:958

dispatchMessage
Handler.java:99

dispatchMessage
MessageQueueThreadHandler. java: 27

Reproducer

https://github.com/Freddy03h/react-native-font-weight-number-android-issue

Screenshots and Videos

github-actions[bot] commented 1 week ago

:warning:	Add or Reformat Version Info
:information_source:	We could not find or parse the version number of React Native in your issue report. Please use the template, and report your version including major, minor, and patch numbers - e.g. 0.70.2

cortinico commented 1 week ago

It's only happen on the old Architecture (no issue with new arch) and with a style created with Stylesheet.create and given to style prop directly, not in an array.

Just a heads up that we're currently hyperfocused on New Architecture, so we won't be able to fix this bug in the immediate future (a community PR is more than welcome though).

Is there a reason why you're not moving to the New Architecture @Freddy03h ?

Freddy03h commented 1 week ago

I have multiple projects for my clients with different dependencies and react-native versions (from 0.71), so it's not always easy to migrate to new architecture, depending on specific native dep on the project.

But we found this bug while creating types for fontWeight in rescript-react-native https://github.com/rescript-react-native/rescript-react-native/pull/806

And since the type number are also added in the Typescript definition, it can happen to Typescript users too: https://github.com/facebook/react-native/blob/8f548be91c2e0ac10e61535154b9db19dad7f3b7/packages/react-native/Libraries/StyleSheet/StyleSheetTypes.js#L814

Also, because it's a weird case, it's not always easy to know why this error happened.

I would like to help but I have no native android skills, also I don't know why this error exist only with Stylesheet.create because now it's supposed to be an identity function, right?

cortinico commented 6 days ago

So the issue is:

java.lang.Double cannot be cast to java.lang.String

I believe we do some props pre-processing if you pass an array vs if you pass a single prop. In this case it being Double cause the framework to fail.

I don't have time to debug this, but for folks interested in helping out, I would put a breakpoint here: https://github.com/facebook/react-native/blob/d1bf828398f81c8bef6537004547a52260af76b4/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.java#L106-L128

and investigate how the array/single-value codepath diverge. The fix is probably trivial

shubhamguptadream11 commented 6 days ago

I am working on fixing this.

Freddy03h commented 6 days ago

I believe we do some props pre-processing if you pass an array vs if you pass a single prop.

It's a little more vicious than this because direct inline style works well

shubhamguptadream11 commented 6 days ago

In StyleSheet.js we are doing Object.freeze() for Dev Mode only. By removing Object.freeze there this issue was resolved. But that code is just for Dev Mode. @Freddy03h Can you confirm if this issue is coming on release builds as well?

For Dev Mode only we are making style object immutable. So Ideally this issue should not come in production.

I checked this on release build as well. It's working fine for me. The issue is only on DEV Mode

shubhamguptadream11 commented 5 days ago

@cortinico Here are my findings Reason: This issue is coming because of we are sending fontWeight as number instead of string from JS to Android.

When we are creating styles using StyleSheet.create() we are calling Object.freeze() on that making styles completely immutable. So when in node_modules/react-native/Libraries/Text/Text.js we are trying to mutate fontWeight with its string conversion it's not get updated. The problem is not just with this props even textAlignVertical props will also not get updated.

Why this is not happening in other cases?

In case of inling styles no freeze method is called.
In case of same style passed as an array. While flattening the object we had created an new object in node_modules/react-native/Libraries/StyleSheet/flattenStyle.js so there wouldn't be any problem.

Solution

Since we are doing Object.freeze() in dev mode only we can remove this from StyleSheet.js file, because now we actually want to reassign few props like fontweight.

Let me know what you think. I can raise the PR for same

Freddy03h commented 5 days ago

@shubhamguptadream11 Effectively I can't reproduce the error on release mode, so it's dev mode only.

And your change https://github.com/facebook/react-native/pull/45299/files fix the issue, thank you!

shubhamguptadream11 commented 5 days ago

@Freddy03h Thanks for checking this. This is not my PR: https://github.com/facebook/react-native/pull/45299/files (As per this PR we are actually doing one extra operation for each style object in release mode as well which will be redundant since issue is not on release mode. And if let's say we put a DEV mode check there then it doesn't make sense to do freeze in StyleSheet file).

So I am waiting for inputs from @cortinico.

meetdhanani17 commented 5 days ago

And

freeze make sense while development we preventing user to modify style and I have added extra layer that also removed on release. that time it check if object is freeze than only its return new object

Share your suggestions into https://github.com/facebook/react-native/pull/45299/files PR @cortinico

javache commented 2 days ago

Addressing in https://github.com/facebook/react-native/pull/45340

meetdhanani17 commented 1 day ago

Addressing in #45340

FlowFixMe it is also there in your PR why you need it. i think its not proper solution of it. @cortinico i think now you have to raised a pr (Please read my 1st PR comments first and provide us feedback, why it's closed without any proper valid reason)

meetdhanani17 commented 1 day ago

@javache you have to do changes in TextInput.js related to styles changes

facebook / react-native