jpudysz / react-native-unistyles

Level up your React Native StyleSheet
https://unistyl.es
MIT License
1.39k stars 39 forks source link

[2.8.0.-rc.1] Possible Unhandled Promise Rejection (id: 1): Error: Exception in HostFunction: Value is an object, expected a number at setColor (native) #214

Closed efstathiosntonas closed 3 months ago

efstathiosntonas commented 3 months ago

Description

Hi @jpudysz,

Just installed 2.8.0 rc1 and it throws this on iOS:

 Possible Unhandled Promise Rejection (id: 1):
Error: Exception in HostFunction: Value is an object, expected a number
Error: Exception in HostFunction: Value is an object, expected a number
    at setColor (native)

has something changed in the API of navigation and status bar setColor or it's leaking in the iOS part?

Will test Android in a while.

Steps to reproduce

  1. install 2.8.0 rc1

Snack or a link to a repository (optional)

No response

Unistyles version

2.8.0-rc.1

React Native version

0.74.2

Platforms

iOS

Engine

Hermes

Architecture

Paper (old)

jpudysz commented 3 months ago

How did you call it from JS part? API was extended a little bit to support alpha channel.

efstathiosntonas commented 3 months ago

seems it's leaking ~on~ to iOS, located the line in the bundle that triggers the rejection

      /**
       * Get the navigation bar info (Android)
       * @returns - The navigation bar api { width, height, setColor }
       */
    }, {
      key: "navigationBar",
      get: function get() {
        var _this3 = this;
        return {
          width: this.unistylesBridge.navigationBar.width,
          height: this.unistylesBridge.navigationBar.height,
          setColor: function setColor(color) {
            return _this3.unistylesBridge.navigationBar.setColor(color != null ? color : '');
          }
        };
      }

I do not check for platform in the code when setting colors, eg.

  useEffect(() => {
    if (isFocused) {
      UnistylesRuntime.navigationBar.setColor('#FF2D79');
      UnistylesRuntime.statusBar.setColor('#E32A8A');
    } else {
      UnistylesRuntime.navigationBar.setColor(theme.colors.neutrals['0']);
      UnistylesRuntime.statusBar.setColor(theme.colors.neutrals['0']);
    }
  }, [isFocused, theme.colors.neutrals]);

Previously the above did not throw rejection error on iOS.

jpudysz commented 3 months ago

Did you expo prebuild ? This is new API:

image

Setting colors should be ignored for iOS. You don't need to check platform

efstathiosntonas commented 3 months ago

I'm on bare rn with expo-modules.

Setting colors should be ignored for iOS. You don't need to check platform

Yeah, that's why I mentioned earlier that it leaks to iOS.

This is the exact line that throws: return _this3.unistylesBridge.navigationBar.setColor(color != null ? color : '');

jpudysz commented 3 months ago

Give me a sec, will try on bare.

efstathiosntonas commented 3 months ago

All good in Android, no rejection

efstathiosntonas commented 3 months ago

~I think something is off in the parseColor, if i just pass this.unistylesBridge.navigationBar.setColor(color, 1) it stops throwing.~

~When I console.log(...parseColor(color, alpha)) it returns undefined~

jpudysz commented 3 months ago

I'm testing it and I have no crash.

Tested with following values:

UnistylesRuntime.navigationBar.setColor('#000000', 0.2)
UnistylesRuntime.navigationBar.setColor('#000000', 1)
UnistylesRuntime.navigationBar.setColor('#000000')
UnistylesRuntime.navigationBar.setColor('#50000000', 0.2)
UnistylesRuntime.navigationBar.setColor('#50000000')
UnistylesRuntime.navigationBar.setColor('red')
jpudysz commented 3 months ago

parseColor always returns tuple 🤔, something is off

efstathiosntonas commented 3 months ago

@jpudysz not crashing, just promise reject. That's really weird.

I console.log the color in the setColor and I always get a color back, discard the parseColor, it returns a tuple just fine, undefined was from another part on my code.

efstathiosntonas commented 3 months ago

This still stands though: .setColor(color, 1) (not a tuple) does not throw rejection, we spread the parseColor and the output is not comma separated

jpudysz commented 3 months ago

The correct code in UnistylesRuntime is:

return this.unistylesBridge.navigationBar.setColor(...parseColor(color, alpha))

I can see it the source code, not sure from where did you get:

return _this3.unistylesBridge.navigationBar.setColor(color != null ? color : '');
efstathiosntonas commented 3 months ago

@jpudysz I got it from metro's bundler. I just reset the metro cache and:

      /**
       * Get the status bar info
       * @returns - The status bar api { width, height, setColor, setHidden }
       */
    }, {
      key: "statusBar",
      get: function get() {
        var _this2 = this;
        return {
          width: this.unistylesBridge.statusBar.width,
          height: this.unistylesBridge.statusBar.height,
          setColor: function setColor(color, alpha) {
            return _this2.unistylesBridge.statusBar.setColor((0, _$$_REQUIRE(_dependencyMap[4], "../utils").parseColor)(color, alpha)); <----- this line
          },
          setHidden: function setHidden(hidden) {
            return _this2.unistylesBridge.statusBar.setHidden(hidden);
          }
        };
      }
jpudysz commented 3 months ago

So, its ok now? Metro cached the UnistylesRuntime? 😁

efstathiosntonas commented 3 months ago

No, still got it, did you read this since I edited it?

jpudysz commented 3 months ago

This is super super weird, it works here as intended. You can spread the arguments from the array...

Nevermind I will destructure it explicitly

efstathiosntonas commented 3 months ago

What kind of sorcery is this haha? This is working fine:

this.unistylesBridge.navigationBar.setColor(...parseColor(...[color, alpha])), haven't tested in Android though.

jpudysz commented 3 months ago

Please try RC 2

efstathiosntonas commented 3 months ago

Works as a charm, we just wasted 1 hour of our Saturday evening because of 3 dots hahaha!

Thank Jacek!

jpudysz commented 3 months ago

I love it! Changed 4k lines of code with no issues, but yet metro doesn't like .... Thanks for your time @efstathiosntonas !

efstathiosntonas commented 3 months ago

Sounds like classic react-native to me 💯

efstathiosntonas commented 3 months ago

@jpudysz went through ios code while hunting this down, is root background color supported on ios or not yet? I saw some references but didn't check if it's bridged or not to js side

jpudysz commented 3 months ago

It is. Same API as Android 🙏

efstathiosntonas commented 3 months ago

amazing! will get rid of expo-system-ui asap 😅

jpudysz commented 3 months ago

Haha, cool!

Let me also know about the Android insets. I've spent a lot of time to get them right 😵‍💫

efstathiosntonas commented 3 months ago

~insets are working fine, commented in the original issue couple of days ago!~