Repository navigation
chore(android): drop deprecated event and package APIs - #1219
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The Android-only React Native migration spans package integration, generated delegate code, and Fabric event APIs, but build and runtime verification across RN 0.86–0.88 is unavailable in this review environment.
No open issues 🎉
What changed in this PR
Updates the Android implementation to use current React Native package, event, and codegen APIs while preserving existing pager behavior.
Changes:
- Migrates package registration from
ReactPackagetoBaseReactPackage. - Updates Android events to use surface-aware constructors and
getEventData(). - Simplifies generated view-manager delegation and non-null callbacks.
- Removes redundant pager configuration defaults.
| File | Description |
|---|---|
android/src/main/java/com/reactnativepagerview/PagerViewViewPackage.kt |
Migrates package registration to BaseReactPackage. |
android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt |
Uses the typed codegen delegate and simplified view-manager callbacks. |
android/src/main/java/com/reactnativepagerview/event/PageSelectedEvent.kt |
Migrates page-selection events to the surface-aware event API. |
android/src/main/java/com/reactnativepagerview/event/PageScrollStateChangedEvent.kt |
Migrates scroll-state events to the surface-aware event API. |
android/src/main/java/com/reactnativepagerview/event/PageScrollEvent.kt |
Migrates scroll events and retains finite-offset sanitization. |
android/src/main/java/com/reactnativepagerview/ComposePagerView.kt |
Supplies surface IDs when dispatching events and removes redundant pager defaults. |
Review effort: Lite
Apex SentinelVerdict: Since the last review: 1 new · 4 still open · 2 no longer raised 5 finding(s): 1 major, 4 nit. Verdict: request_changes. Required fixes
🟠 Major (1)
🔵 Nit (4)
Reply under an inline comment with Earlier reviewsReview of
|
| Where | Finding |
|---|---|
android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt:15 |
receiveCommand override deleted — delegate no longer forwards commands (setPage / setPageWithoutAnimation / setScrollEnabledImperatively) |
android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt:111 |
keyboardDismissMode prop is now a silent no-op on Android; JS dismisses it only from onPageScroll |
🔵 Nit (4)
| Where | Finding |
|---|---|
android/src/main/java/com/reactnativepagerview/ComposePagerView.kt:55 |
surfaceId captured once in the constructor from a cast that can throw |
android/src/main/java/com/reactnativepagerview/ComposePagerView.kt:649 |
reverseLayout = false removals are unrelated to this PR's scope |
android/src/main/java/com/reactnativepagerview/event/PageScrollStateChangedEvent.kt:18 |
canCoalesce() = false added for PageScrollStateChangedEvent only; PageScrollEvent still coalesces |
android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt:106 |
No-op prop stubs carry inconsistent one-line comments that point outside the module |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on b0c6058. The verdict and the full list are in the summary comment.
| @@ -19,18 +19,15 @@ class PagerViewViewManager : ViewGroupManager<ComposePagerView>(), RNCViewPagerM | |||
| const val NAME = "RNCViewPager" | |||
There was a problem hiding this comment.
🟠 Major · Removed receiveCommand override may stop JS commands from reaching the delegate
The PR deletes the explicit override fun receiveCommand(root, commandId, args) { mDelegate.receiveCommand(root, commandId, args) }, leaving command dispatch to whatever the base class does with getDelegate(). Every command this library exposes goes through that path — Commands.setPage, setPageWithoutAnimation and setScrollEnabledImperatively are called from src/PagerView.tsx:93/98/106 — and the delegate (RNCViewPagerManagerDelegate) is the only thing that maps a command name string to setPage/setPageWithoutAnimation/setScrollEnabledImperatively. If the RN version's ViewGroupManager/BaseViewManager.receiveCommand does not forward to getDelegate().receiveCommand(...) (community view managers have historically had to write exactly that override, which is why it was there), all three imperative methods become silent no-ops: no crash, no error, ref.setPage(3) just does nothing. This is the more worrying case because commit ce0d489 (#1212) just fixed setPage timing, i.e. this path is known-fragile. I could not verify the RN 0.88 base-class behaviour in this checkout (no node_modules / RN sources available), so confidence is not full — but the merge should be gated on an actual run of the example app's setPage/setPageWithoutAnimation/setScrollEnabledImperatively (e2e covers these) rather than on the Kotlin compiling.
Suggested change
Either keep the explicit receiveCommand override delegating to delegate.receiveCommand(root, commandId, args), or confirm against the RN version in example (0.88.0) that the base manager forwards commands through getDelegate(), and run the example/e2e suite exercising setPage, setPageWithoutAnimation and setScrollEnabledImperatively.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -648,7 +649,6 @@ class ComposePagerView(context: Context) : FrameLayout(context) { | |||
| modifier = Modifier.fillMaxSize(), | |||
There was a problem hiding this comment.
No longer raised on 58496ba.
🔵 Nit · reverseLayout = false removals are unrelated to this PR's scope
Dropping the explicit reverseLayout = false in both HorizontalPager calls is a no-op (the default is false), but it is a Compose API change bundled into a PR whose title is about deprecated event/package APIs; it also silently changes behavior on any Compose Foundation version where the parameter has no default. Split it into its own commit so a bisect over Android event regressions does not land on it.
Suggested change
Move the two reverseLayout = false removals into a separate commit (or keep them and mention them in the PR description).
Details
persona: maintainability, senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| view.setOverScrollMode(value) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
No longer raised on 58496ba.
🔵 Nit · No-op prop stubs carry inconsistent one-line comments that point outside the module
setOverdrag and setKeyboardDismissMode keep their @ReactProp annotations and gained only an explanatory comment ("iOS only." / "Dismissed from JS on Android, see PagerView.tsx.") instead of the no-op bodies being removed. That is fine for JS compat, but the two comments use different conventions for the same kind of note and neither states what a caller should expect if they do pass the prop on Android; a reader grepping overdrag in the Android module now gets a one-line pointer to a TS file in another package. A short comment naming the JS-side file path (or a link) makes the stub self-explanatory.
Suggested change
Use one comment style for both Android no-op props and point at the file that already handles the behaviour (lib/src/PagerView.tsx) rather than a bare "see PagerView.tsx".
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
🟢 e2e: 46 passedAll 46 tests in 19 files
e2e 0.21.3 · 7m 16s · android · run artifacts |
There was a problem hiding this comment.
Apex Sentinel: 1 new finding and 2 moved to a new line on 58496ba. The verdict and the full list are in the summary comment.
|
|
||
| // The package only provides a view manager. | ||
| override fun getModule(name: String, reactContext: ReactApplicationContext): NativeModule? = null | ||
|
|
There was a problem hiding this comment.
🆕 🔵 Nit · getReactModuleInfoProvider() returns an empty map, bypassing generated module info
PagerViewPackage now extends BaseReactPackage but overrides getReactModuleInfoProvider() to return ReactModuleInfoProvider { emptyMap() }. RN's BaseReactPackage implements this by returning the annotation-processor-generated ReactPackageSupport.getReactModuleInfo(...) map, so this override throws away generated metadata for any @ReactModule-annotated class in this package (PagerViewViewManager carries @ReactModule(NAME)) and will silently swallow the metadata of every module a future version adds here — the module lookup/info path has to be updated by hand, and the empty map is exactly the kind of thing that drifts. Returning the inherited implementation (i.e. deleting this override) keeps the package module-free today and stays correct later.
Suggested change
Drop the getReactModuleInfoProvider() override and rely on BaseReactPackage's generated ReactPackageSupport-backed implementation; only supply a custom provider if a native module is actually added to this package.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
Summary
Test Plan
What's required for testing (prerequisites)?
What are the steps to reproduce (after prerequisites)?
Compatibility
Checklist
README.md