Repository navigation
fix(react-native): bugs reported from ng-native - #77
Open
NathanWalker wants to merge 15 commits into
Open
NathanWalker wants to merge 15 commits into
NathanWalker wants to merge 15 commits into
Conversation
…when it matches its last one Fabric keeps _props across prepareForRecycle while the container is cleared, so diffing against _props skipped setNativeViewHandle when a recycled host was mounted with the handle it last showed, leaving it empty. Diff against the oldProps argument instead, which is null on a fresh mount.
react-native-worklets shares host objects across runtimes, so a native object returned from runOnUI is finalized by the JS runtime after the UI runtime is gone. Its destructor released the UI bridge's expandos and caches into the dead runtime, crashing every JS reload (EXC_BAD_ACCESS in NativeApiBridge::forgetObjectExpandos during HadesGC::finalizeAll). The bridge's API host object now marks the bridge torn down as its runtime is finalized; from then on Values the bridge drops are leaked.
install() returned early on a process-wide flag, so after a reload the new runtime had none of NativeScript's globals while isInstalled() reported true. The old com.tns.Runtime also stayed subscribed to GC notifications and released handles into the destroyed jsi::Runtime. - isInstalled()/install() are per host jsi::Runtime. - invalidate() detaches: releases every handle while the runtime is still alive, unregisters it, and resets main-runtime state so the next attach binds to the new JS thread. Metadata is built once per process. - GC notifications for a detached runtime are no-ops at every layer. - A Java callback into JavaScript that was reloaded away logs a warning and returns a default instead of throwing on the calling thread.
Every key of an enum object became a global, including the short member
names (Center, Fill, CloudKit's Zone). Libraries that feature-detect a
global by name were misled: Angular's zoneless scheduler checks
typeof Zone and crashed at bootstrap after init({ globals: true }).
Only full C constant names (NSTextAlignmentCenter, NSOrderedAscending,
kCLAuthorizationStatusRestricted) are defined as globals now, in both
the JS installGlobals() and the shared global installer used by the
Worklets UI runtime. Short names stay on their enum.
- The TurboModule is looked up with TurboModuleRegistry.get, so importing the package in Expo Go or a build made before it was added no longer raises an uncatchable invariant. isAvailable() reports whether the module is present; init() and the other functions that need it throw an error saying to rebuild. - init() no longer installs into a Worklets runtime on Android, where there is none, so it returns true instead of throwing a message about missing react-native-worklets setup. runOnUI()/installWorklets() on Android say they are iOS only. - README: the Android example uses NativeScript.init(), and the threads section states that Android views cannot be touched from JavaScript.
Without strictGlobal the Worklets plugin captures every name a worklet
does not declare into its closure, read on the JS thread when the module
is evaluated, where NativeScript's classes do not exist: a module-level
worklet naming UILabel failed at startup with
"ReferenceError: Property 'UILabel' doesn't exist".
The config plugin and configure CLI now write
['react-native-worklets/plugin', { strictGlobal: true }] and
['babel-preset-expo', { worklets: false }] (so the preset adds no second
copy), upgrade bare entries in an existing config, and warn when an
entry with options lacks strictGlobal. The Babel setup also runs on
Android-only prebuilds. Documented in the README.
….extend - The emitter wrote nullable callbacks as `() => void | null`, which TypeScript reads as a function returning void | null, so passing null (e.g. presentViewControllerAnimatedCompletion's completion) was a type error. They are parenthesized now. - extend() works on every native class at runtime but was undeclared. NativeObject.extend types overridden methods from the class, accepts protocol/exposed methods, and takes objc.ExtendOptions. - README: Android declarations are generated by nativescript.gradle by default from the app's classpath; @nativescript/types-android is not needed.
The property-getter, round-trip and expando lookup caches identified a bridge by its address. After a JS reload a new bridge can be allocated where a destroyed one lived and match its entries: the getter cache then returned a raw pointer into the freed bridge (EXC_BAD_ACCESS in preparedObjCInvocationIsInit on the next launch), and cached misses could hide values that exist.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…eact-native-reported-issues
Put requirements and installation first and move the monorepo build
scripts to a closing section. Document that iOS JS-thread globals are
opt-in via init("", { globals: true }), Android release metadata
filtering, and the Android steps of configure and the Expo plugin.
Fix examples that touched UIKit or undefined names off the UI runtime.
…act-native.json The config plugin and nativescript-rn configure replaced the whole reactNative.metadata block, so every prebuild or configure run deleted a hand-written metadata.android.include. That list is what keeps classes, including autolinked module packages, in filtered Android release metadata. Merge into the existing metadata instead, accept metadata.android.include in the plugin options, and write the file on Android-only prebuilds too. Document the include list in the README.
…nfigure nativescript.gradle generates them unless -PnsGenerateTypings=false, so the next-steps hint no longer calls them on demand or passes =true.
A guest's JS thread is not the main thread, so no JavaScript could touch an Android view. The main thread now borrows the jsi::Runtime while the JS thread waits, as React Native's executeSynchronouslyOnSameThread does: - Java->JS callbacks made on the main thread (a view's listeners) run there instead of being posted to the JS thread. - __nativeScriptRunOnMainThread(fn) runs fn on the main thread from the JS thread, which lends the runtime until it returns. - A JS thread parked inside a Java->JS call releases the EngineHost lock for the borrower and takes it back after. - Callbacks delivered as plain looper messages drain microtasks, which no React Native task would otherwise do. Also installs __nativeScriptAndroidViewHandle, __nativeScriptAndroidContext and __nativeScriptIsMainThread for the JS API.
…View The Fabric component was registered on Android with no view manager. It now hosts the view its nativeViewHandle names, sized to the host, moving it from any previous host.
…ndroid runOnUI runs its callback on the Android main thread instead of throwing. nativeViewHandle(view) names a view for NativeScriptUIView on both platforms, androidContext() is the Context to build a view with, and isMainThread() answers on Android. The README's Android section describes the threading model and how to show a view.
ammarahm-ed
requested changes
Oct 24, 2026
ammarahm-ed
left a comment
ammarahm-ed
left a comment
Collaborator
There was a problem hiding this comment.
Several of the bug fixes are real and should land. But the Android main-thread work added in the later commits takes the wrong approach and needs to come out of this PR completely.
The Android main-thread work
The PR description says Android main-thread support is out of scope and needs real work. The later commits then add it anyway, as a workaround that hands React Native's JS runtime back and forth between the JS thread and the main thread.
That isn't how we do it on iOS, and we don't want a second, incompatible model. On iOS:
- There are two separate runtimes, each with its own NativeScript instance: React Native's JS runtime on the JS thread, and the Worklets UI runtime on the main thread.
- All UI work happens inside worklets on the UI runtime.
- Objects never cross between the runtimes. Views go back to JS as handles, and values are copied.
Sharing one runtime between two threads, as this PR does, causes real problems:
- It releases engine locks in the middle of calls that are still running.
- It runs JS outside React Native's scheduler, which is why the PR has to drain microtasks and report errors by hand.
- It adds new ways to deadlock, on the path of every main-thread callback.
- It makes
runOnUIbehave completely differently on Android than on iOS.
Please remove all of it. That means the main-thread handoff, the private globals it installs, Android runOnUI, and the Android view manager and view registry. The README section describing them goes too.
Android view support should be its own effort, built on a Worklets UI runtime the same way iOS is. Let's agree on that design in an issue before writing code. None of the current approach carries over.
Other things to fix
- Reload works by leaking. Each reload deliberately leaks the old runtime and its values, so memory grows every time. The iOS teardown signal comes from a garbage-collector finalizer, which is unreliable in exactly the case it's meant to fix. Teardown should be driven by the runtime actually being destroyed.
- Calls into a reloaded runtime silently return empty values. That turns a clear error into confusing failures elsewhere. Either fail loudly or stop those callbacks from firing.
- Enum globals are chosen by a guess based on names. The bridge already knows which names are the real C constant names, so the decision should be made there, in one place.
- There's Expo-specific special-casing in core runtime code. It silently renames a Java package. If we need it, it should be explicit and documented.
- Some fixes only land in one of the two runtime trees. The other tree has the same bugs. Fixes need to go into both.
- More process-wide state makes multiple React instances harder, not easier.
Process
- A large reformat of an unrelated file is mixed into the riskiest change. Please drop it.
- Unrelated CI changes shouldn't be part of a bug-fix PR.
- The new tests check the source text against mocks rather than real behaviour. Threading changes need device testing; the emulator isn't reliable for that.
What can stay
Once the main-thread work is removed and the points above are addressed, these are good to land:
- The iOS recycled-view fix
- The bridge cache fix
- Re-attaching on Android after a reload
- The Worklets Babel config
- Importing the package without its native module
- The typings fixes
- Keeping the Android metadata config across runs
- The README rewrite, minus the Android threading section
Collaborator
|
Also you should not be publishing new versions of packages on npm and announcing support for android views has landed when the PR has not been reviewed or even looked into once. We haven't even agreed on the API yet as you can see in the review above and there is already a published version with this change? This is the runtime and we have to be extra careful. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
@nativescript/react-nativebugs reported in the "NativeScript bugs" section of ng-native/ng-native#931. The numbers below match that list.Fixes
1. iOS: a recycled
NativeScriptUIViewstayed empty when remounted with its last handle.updatePropsdiffed against_props, which Fabric keeps acrossprepareForRecyclewhile the container is cleared. It now diffs against theoldPropsargument.RCTMountingManagerpassesnullptron a fresh mount, which maps to the default props._propsis deliberately left alone, because RN's ownprepareForRecyclediffs layer state against it.2. iOS: a native object returned from a worklet crashed the next JS reload. react-native-worklets shares host objects between runtimes, so the JS runtime finalizes the UI bridge's host object after the UI runtime is gone, and the bridge then released
jsi::Values into a dead runtime.ObjCBridge.mmidentified a bridge by its memory address. A bridge allocated where a destroyed one used to live matched that bridge's cached entries, and the property-getter cache holds raw pointers into it. Each bridge now has a process-unique ID instead.3. Android: after a JS reload, NativeScript said it was installed but the new runtime had none of its globals.
install()andisInstalled()now work per hostjsi::Runtimeinstead of using a process-wide flag.invalidate()detaches the old runtime. RN 0.86 calls it after the JS thread has stopped and before the runtime is destroyed.4. Android: SIGSEGV in NativeScript's GC after a reload.
notifyGcFastno longer throws on the GC listener's thread.postDelayedRunnable) now logs a warning and returns the default for its return type, instead of calling into the destroyed runtime.5. The Worklets Babel plugin was not configured with
strictGlobal.nativescript-rn configurenow write['react-native-worklets/plugin', { strictGlobal: true }]and['babel-preset-expo', { worklets: false }].strictGlobal.6.
init("", { globals: true })made every enum member a bare global (Zone,Center). Only full C constant names become globals now, for exampleNSTextAlignmentCenter,NSOrderedAscendingandkCLAuthorizationStatusRestricted. Short names stay on their enum. The same rule applies to the JSinstallGlobals()and to the shared global installer that the Worklets UI runtime uses. It is consistent with the runtime'sEnumConstantsAsGlobalVariablestest.7. Android:
init()threw a misleading "could not install into the Worklets UI runtime".init()no longer tries the Worklets install on Android, so it returnstrue.runOnUI()andinstallWorklets()on Android now say they are iOS only.8. Importing the package without its native module raised an error no
try/catchcould catch. The module is looked up withTurboModuleRegistry.get.NativeScript.isAvailable()reports whether the module is present.isInstalled()returnsfalsewhen it isn't.init()and the other functions that need the module throw an error saying to rebuild.9. Generated typings.
(() => void) | null.NativeObject.extendis declared, withobjc.ExtendOptions.nativescript.gradlegenerates them from the app's classpath by default, so@nativescript/types-androidis not needed.Also in this PR
README rewritten for npm readers.
0.0.1tarball steps are gone.init("", { globals: true }). The examples that touched UIKit or undefined names off the UI runtime are fixed.metadata.android.include, and the Android steps ofconfigureand the Expo plugin.metadata.android.includewas deleted by every prebuild andconfigurerun.ensureMetadataConfigreplaced the wholereactNative.metadatablock innativescript.react-native.json. That list is the only way to keep a class in filtered Android release metadata when the bundle analysis cannot see it. It is also the only way to keep autolinked module packages, because the release blacklist beatswhitelist.mdg.metadata.android.includein its options.nativescript-rn configuresaid Android typings were generated on demand.nativescript.gradlegenerates them by default, so the hint no longer passes-PnsGenerateTypings=true.Not in this PR
10. Android has no Worklets runtime, so Android views can't be touched. This needs real work, not a patch:
WorkletRuntimeHolder, scheduled onto the main thread.The README now states the limitation. A follow-up could add a smaller "run on main and wait" call.
Also left for follow-ups:
runOnUIand then used on the JS thread still runs through the UI bridge. This PR only makes teardown safe; re-wrapping host objects as they cross runtimes would be the full fix.Testing
superviewis set.UIViewis held in JS, with no crash reports.ZoneandCenterareundefinedon both the JS runtime and the UI runtime, and full names are numbers.UILabelloads.isInstalled() === false,install()works, andtypeof java === 'object'.Runnableand aHandler.Callbackposted before a reload are dropped with a warning after it, and a callback whose runtime is still alive still runs.tsc --strictagainst freshly generated iOS 27.1 declarations coveringpresentViewControllerAnimatedCompletion(vc, true, null)andextend(). They fail before this branch and pass with it.config-pluginandenum-member-globalscases. Theconfig-plugincases include a hand-writtenmetadata.androidblock surviving a run and plugin options setting it; both fail before this branch.invalidate()path for it posts the detach to the JS thread.