-
Notifications
You must be signed in to change notification settings - Fork 48
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
Async node android #469
Async node android #469
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #469 +/- ##
=======================================
Coverage 91.94% 91.94%
=======================================
Files 340 340
Lines 26838 26838
Branches 1946 1946
=======================================
+ Hits 24675 24677 +2
+ Misses 2149 2147 -2
Partials 14 14 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly small things - feel free to ignore Nit
s if you disagree or think it's scope creep. Also looks like //jvm/core:lint
test is failing, but should be fixable with bazel run //jvm/core:lint-fix
(or ./tools/lint-kotlin
to apply on all Kotlin targets).
The only other thing that I see missing in this PR is the ported PromiseTest
changes:
@TestTemplate
fun testPromiseConstructor() = runBlockingTest {
runtime.execute(
"""
class TestClass {
name = 'foo';
constructor(promise) {
promise.then((value) => {
this.name = value;
});
};
}"""
)
val promise = runtime.Promise<String> { jsRes, _ ->
val result = "bar"
jsRes(result)
}
runtime.add("myPromise", promise)
val value = runtime.execute("new TestClass(myPromise)") as Node
promise.toCompletable(String.serializer()).await()
assertEquals("bar", value["name"])
}
Added the ported test
jvm/core/src/main/kotlin/com/intuit/playerui/core/bridge/hooks/NodeHook.kt
Outdated
Show resolved
Hide resolved
jvm/core/src/main/kotlin/com/intuit/playerui/core/bridge/hooks/NodeAsyncParallelBailHook1.kt
Show resolved
Hide resolved
.../async-node/jvm/src/test/kotlin/com/intuit/playerui/plugins/asyncnode/AsyncNodePluginTest.kt
Show resolved
Hide resolved
.../async-node/jvm/src/test/kotlin/com/intuit/playerui/plugins/asyncnode/AsyncNodePluginTest.kt
Outdated
Show resolved
Hide resolved
366310b
to
554861f
Compare
This PR accommodates to this requirement #459 i.e Port internal async node plugin implementation to android
Change Type (required)
Indicate the type of change your pull request is:
Added a new component as per the following plugins/async-node
├── core
│ ├── BUILD
│ ├── src
│ └── package.json
└── jvm
├── BUILD
├── deps.bzl
├── src/main/kotlin/com/intuit/playerui/plugins/asyncnode
│ └── AsyncNodePlugin.kt
└── src/test/kotlin/com/intuit/playerui/plugins/asyncnode
└── AsyncNodePluginTest.kt
All tests are passing in local :
patch
minor
major
Does your PR have any documentation updates?
Version
Published prerelease version:
0.9.0-next.2
Changelog
Release Notes
Storybook Addon Fixes (#449)
[Hermes] Android integration (#410)
Initial integration with the Hermes JavaScript runtime. This shows a tremendous size improvement over the existing J2V8 integration of ~70% (7.6 MB -> 2.3 MB, architecture dependent).
Opt-in
For now, the default runtime integration provided by the Android Player will still be
com.intuit.playerui:j2v8-android
, but Hermes can be opted in manually by excluding the J2V8 transitive dependency and including the Hermes artifact:Most of the setup for this integration is done simply by including the right dependency (and excluding the wrong one), however, the
hermes-android
integration also relies on the SoLoader for loading the native libraries. All that's needed is to initialize theSoLoader
(should be on your classpath with thehermes-android
dependency) with an AndroidContext
somewhere before you use theAndroidPlayer
, potentially in your activitiesonCreate
:🚀 Enhancement
🐛 Bug Fix
Authors: 4