Skip to content
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

Upgrade JSC past 260345 #159

Open
jzxchiang1 opened this issue Oct 17, 2021 · 19 comments
Open

Upgrade JSC past 260345 #159

jzxchiang1 opened this issue Oct 17, 2021 · 19 comments

Comments

@jzxchiang1
Copy link

Issue Description

Unfortunately, many crypto apps rely on BigInt in JS. But WebKit didn't enable BigInt support until this changeset, which is almost a year after the currently released JSC version (i.e. 250230) was initially released.

The current version WebKitGTK 2.26.1 is from 2019. Is it possible to upgrade JSC to a more recent version?

I spoke with @Kudo and apparently he tried to update it to WebKitGTK 2.30.5, which was released in early 2021. That didn't work because:

"there was a breaking change that jsc removed the Intl toggle. so we probably would have to ship only the Intl variant and increase apk size."

Is there a way to get this to work, or alternatively to use an earlier version of WebKitGTK that doesn't have this breaking change, e.g. late 2020? Not sure when that change was introduced.

Thank you! This would be huge for the crypto space.

Version, config, any additional info

@jzxchiang1
Copy link
Author

Bump, I'm wondering if this is possible, thanks!

@jzxchiang1
Copy link
Author

Bump

@cortinico
Copy link
Member

Have you considered using Hermes instead? Would that address your specific problem?

@ls-andrew-goodale
Copy link

It looks like Hermes doesn't support BigInt yet either.
facebook/hermes#510

@cortinico
Copy link
Member

It looks like Hermes doesn't support BigInt yet either. facebook/hermes#510

I see your point. I guess your best bet would be to reach out to someone at Expo and try to prioritize the whole BigInt support.

@ls-andrew-goodale
Copy link

The ICU library has a build tool to reduce the size of the ICU data file, and make smaller libraries for embedded use cases.
https://unicode-org.github.io/icu/userguide/icu_data/buildtool.html#overview-what-is-in-the-icu-data-file
Maybe this can be added to this project to make a smaller library for the "non-intl" build of JSC? @Kudo what do you think?

@jzxchiang1
Copy link
Author

https://unicode-org.github.io/icu/userguide/icu_data/buildtool.html#overview-what-is-in-the-icu-data-file

Yeah I would use Hermes if it supported:

  • BigInt
  • WebAssembly

That's my only blocker. Can you explain why Expo would need to prioritize BigInt support? I thought Facebook was maintaining the Hermes engine. Thanks.

@Kudo
Copy link
Member

Kudo commented Jan 21, 2022

The ICU library has a build tool to reduce the size of the ICU data file, and make smaller libraries for embedded use cases. https://unicode-org.github.io/icu/userguide/icu_data/buildtool.html#overview-what-is-in-the-icu-data-file Maybe this can be added to this project to make a smaller library for the "non-intl" build of JSC? @Kudo what do you think?

@ls-andrew-goodale there're still some difference in the non-intl variant, e.g. typeof(Intl) === 'undefined'. i still think about to ship intl variant only. maybe we can do that after hermes be the default vm in react-native.

@sunnylqm
Copy link

sunnylqm commented Jun 5, 2022

An updated jsc runtime would be very much appreciated by current rn/expo web3 developers, given that jsc/hermes are hardcoded in some important deps, which makes it really difficult to adopt the juicy v8

@oblador
Copy link
Member

oblador commented Jun 12, 2022

What about releasing it as a pre-release or simply not bump the dependency in React Native? Hermes is missing some pretty basic JS features like block scoping and classes and Meta is consistently rejecting those as valid use cases, making it a non-viable option for us. It would be great to make use of the latest improvements to JSC on an opt-in basis.

@Kudo
Copy link
Member

Kudo commented Jun 13, 2022

i can publish a new release on npm, just not to bump the version in react-native.

@sunnylqm i've seen you have a version already. do you mind to send us a pr?

@sunnylqm
Copy link

sunnylqm commented Jun 13, 2022

@Kudo I tried with webkitgtk version 2.27, 2.28 with some patches commented out, but unfortunately there are some crash seen in production app.

@sunnylqm
Copy link

sunnylqm commented Jun 13, 2022

With almost zero knowledge on c/c++, i am not capable and lucky enough to build version above 2.28.4 😂

@Kudo
Copy link
Member

Kudo commented Jun 13, 2022

well, i can try to do the upgrade. no promise yet because i'm super busy these two months.

@sunnylqm
Copy link

#168
enable bigint support for the current jsc. it kinda works if you do not require much features.

@Kudo
Copy link
Member

Kudo commented Jul 13, 2022

close via #169. jsc-android@next, please note that there're some breaking changes from this version. let me know whether it works for you. thanks!

@antondalgren
Copy link

Was #169 supposed to add support for bigint? @Kudo

@antondalgren
Copy link

I tried with a fresh install of react-native 0.69.10 and setting following resolution to my package.json.

  "resolutions": {
    "jsc-android": "294992.0.0"
  },

But the android version still doesn't work with bigint for me.

@antondalgren
Copy link

Stumbled upon this comment, facebook/react-native#35504 (comment) and it made it work for me.

leotm added a commit to MetaMask/metamask-mobile that referenced this issue May 2, 2023
- react-native-community/jsc-android-buildscripts#159 (comment)
- facebook/react-native#35504 (comment)
- vanilla RN 0.71.6 tested working: 1n+2n, BigInt(1)+BigInt(2)
- org.webkit:android-jsc-intl:+ required (which we're already using)
- package.json>resolution not required (result: resolution field incompatible)

Should prevent need for further pos BE patches till RN 0.71.6 upgrade
e.g. #6305 cc @Cal-L

But 294992.0.0 results in repo-specific dep build error to resolve (pos more):

> Configure project :react-native-reanimated
No AAR for react-native-reanimated found. Attempting to build from source.
Android gradle plugin: 4.2.2
Gradle: 6.9

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.9/userguide/command_line_interface.html#sec:command_line_warnings
Error: Command failed: ./gradlew app:installProdDebug -PreactNativeDevServerPort=8081
OpenJDK 64-Bit Server VM warning: Ignoring option MaxPermSize; support was removed in 8.0
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/03 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/03 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/03 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/leo/Documents/GitHub/metamask-mobile/node_modules/react-native-reanimated/android/build.gradle' line: 1059

* What went wrong:
A problem occurred evaluating project ':react-native-reanimated'.
> Expected directory '/Users/leo/Documents/GitHub/metamask-mobile/node_modules/react-native/../jsc-android/dist/org/webkit/android-jsc' to contain exactly one file, however, it contains no files.
leotm added a commit to MetaMask/metamask-mobile that referenced this issue May 2, 2023
- react-native-community/jsc-android-buildscripts#159 (comment)
- facebook/react-native#35504 (comment)
- vanilla RN 0.71.6 tested working: 1n+2n, BigInt(1)+BigInt(2)
- package.json>resolution not required (result: resolution field incompatible)
- org.webkit:android-jsc-intl:+ required (which we're already using)

294992.0.0 prevents need for e.g. `big-integer` shim, till RN 0.71.6 shipped
#6221

294992.0.0 prevents need for BE patches, till RN 0.71.6 upgrade shipped
e.g. #6305 cc @Cal-L

But (the catch) 294992.0.0 results in repo-specific dep build error,
possibly more deps to resolve, likely via patch(es):

> Configure project :react-native-reanimated
No AAR for react-native-reanimated found. Attempting to build from source.
Android gradle plugin: 4.2.2
Gradle: 6.9

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.9/userguide/command_line_interface.html#sec:command_line_warnings
Error: Command failed: ./gradlew app:installProdDebug -PreactNativeDevServerPort=8081
OpenJDK 64-Bit Server VM warning: Ignoring option MaxPermSize; support was removed in 8.0
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/03 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/03 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/03 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/leo/Documents/GitHub/metamask-mobile/node_modules/react-native-reanimated/android/build.gradle' line: 1059

* What went wrong:
A problem occurred evaluating project ':react-native-reanimated'.
> Expected directory '/Users/leo/Documents/GitHub/metamask-mobile/node_modules/react-native/../jsc-android/dist/org/webkit/android-jsc' to contain exactly one file, however, it contains no files.
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

No branches or pull requests

7 participants