mrousavy / nitro

🔥 Insanely fast native C++, Swift or Kotlin modules with a statically compiled binding layer to JSI
https://nitro.margelo.com
MIT License
642 stars 22 forks source link

fix: Call `ordinal()` method instead of trying to access field which doesn't exist #338

Closed mrousavy closed 3 days ago

mrousavy commented 5 days ago

Previously, we tried to access .ordinal as a field on an enum.

Apparently this doesn't exist, and ordinal() is a method on an enum. So this PR now changes this to call that method.

One thing I am wondering is; how did this work before? Afaik we have those test cases... 🤔

  1. We get an enum from a property here: https://github.com/mrousavy/nitro/blob/d76d0c02621d4183544d2a8af5b8b334318b4551/packages/react-native-nitro-image/src/specs/TestObject.nitro.ts#L52
  2. We get an array of enums here: https://github.com/mrousavy/nitro/blob/d76d0c02621d4183544d2a8af5b8b334318b4551/packages/react-native-nitro-image/src/specs/TestObject.nitro.ts#L64
  3. We get an enum from a method here: https://github.com/mrousavy/nitro/blob/d76d0c02621d4183544d2a8af5b8b334318b4551/packages/react-native-nitro-image/src/specs/TestObject.nitro.ts#L80

cc @Szymon20000 - didn't those tests work? In the example app they're green...?

vercel[bot] commented 5 days ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **nitro-docs** | ⬜️ Skipped ([Inspect](https://vercel.com/margelo/nitro-docs/DFdJXhaQGVvjg7WwTgtxfbh7q2jZ)) | | | Nov 16, 2024 11:38pm |
mrousavy commented 3 days ago

hm - I just tested this again (in https://github.com/mrousavy/nitro/pull/346), and it seems like getFieldValue("ordinal") works as expected for me. Again, this would've failed in the tests already, we use this property.

So I think this is not a bug - maybe there was something else that was broken in your app @Szymon20000.

image

mrousavy commented 3 days ago

Ah there's why this works: image

In Kotlin, enums have a ordinal field. In Java, enums have a ordinal() method.

Since we use Kotlin, we're good.

shovel-kun commented 2 days ago

Not sure why but I keep getting the same no "I" field "ordinal" in class error when using Kotlin Enums. The solution in this PR fixed it for me.

mrousavy commented 2 days ago

@shovel-kun which Kotlin version are you using? This is weird, ordinal is definitely a field in my test cases. It works in the example app.

mrousavy commented 2 days ago

@shovel-kun can you please test this PR? https://github.com/mrousavy/nitro/pull/360

This is a better solution in my opinion since it has no performance impact. The PR here has one, as we switch from field access to method call.

shovel-kun commented 2 hours ago

@mrousavy

Tested the PR and it is working for me.

Not sure how to check what Kotlin version I'm using, I think 2.0.0

    classpath "com.android.tools.build:gradle:7.2.1"
    classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:2.0.21"
    classpath "org.jetbrains.kotlin:kotlin-serialization:2.0.21"

    …

    apply plugin: "com.android.library"
    apply plugin: 'org.jetbrains.kotlin.android'
    apply from: '../nitrogen/generated/android/Tachiyomi+autolinking.gradle'
mrousavy commented 2 hours ago

great, thx