Skip to content

feat: [SDK-4768] support disabling location module#29

Merged
fadi-george merged 2 commits into
mainfrom
fadi/sdk-4768
Jun 11, 2026
Merged

feat: [SDK-4768] support disabling location module#29
fadi-george merged 2 commits into
mainfrom
fadi/sdk-4768

Conversation

@fadi-george

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Adds ONESIGNAL_DISABLE_LOCATION support so Capacitor apps can exclude the native OneSignal location module from Android and iOS builds.

Details

Motivation

Apps that do not use OneSignal.Location should be able to remove the native location module, matching the Flutter SDK behavior and reducing unnecessary native dependency surface.

Scope

This PR updates Android Gradle, CocoaPods, and SwiftPM dependency resolution to omit location when ONESIGNAL_DISABLE_LOCATION=true or 1 is set. It keeps the JavaScript API stable and makes Android location bridge calls no-op safely when the location module is missing.

Testing

Unit testing

Existing TypeScript bridge tests were run; no new unit tests were added because the public TypeScript API is unchanged and the behavior is native dependency-resolution/runtime fallback logic.

Manual testing

  • bun run lint
  • bun run test
  • bun run build
  • ONESIGNAL_DISABLE_LOCATION=true Android dependency resolution confirmed modular core, notifications, and in-app-messages artifacts
  • ONESIGNAL_DISABLE_LOCATION=true ./gradlew :onesignal-capacitor-plugin:compileDebugKotlin
  • ONESIGNAL_DISABLE_LOCATION=true swift package describe --type json confirmed OneSignalLocation is omitted
  • ONESIGNAL_DISABLE_LOCATION=true pod ipc spec OneSignalCapacitorPlugin.podspec confirmed location-free CocoaPods subspecs

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

Made with Cursor

Co-authored-by: Cursor <cursoragent@cursor.com>
@fadi-george fadi-george requested a review from a team June 10, 2026 18:15
@sherwinski

Copy link
Copy Markdown

@claude review

@sherwinski

Copy link
Copy Markdown

@claude review once

@fadi-george fadi-george merged commit 31a566d into main Jun 11, 2026
5 checks passed
@fadi-george fadi-george deleted the fadi/sdk-4768 branch June 11, 2026 00:15
Comment on lines +650 to +654
try {
OneSignal.Location.requestPermission()
} catch (t: Throwable) {
logLocationModuleNotAvailable(t)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the scope is cancelled while OneSignal.Location.requestPermission() is suspended, the new catch (t: Throwable):

  • catches the CancellationException instead of letting cancellation propagate,
  • logs a misleading "location module is not available" error,
  • then calls call.resolve() on the destroyed bridge — the exact crash path the previous fix removed.

Fix: rethrow cancellation before the broad catch:

try {
    OneSignal.Location.requestPermission()
} catch (e: CancellationException) {
    throw e
} catch (t: Throwable) {
    logLocationModuleNotAvailable(t)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each try/catch in this file also throws when the SDK isn't initialized yet, which can be confusing to debug. Consider a message like "OneSignal.Location call failed — the location module may not be included in this build" or detecting the module-missing case specifically.

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

Successfully merging this pull request may close these issues.

3 participants