-
Notifications
You must be signed in to change notification settings - Fork 11
fix(ios): added timeout implementation for both getCurrentPosition and watchPosition #55
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
Conversation
|
@OS-ruimoreiramendes per what we talked earlier this should only go with Capacitor 8, in which case we should point this PR to |
@OS-pedrogustavobilro Already changed the base branch to next and fix the conflicts |
packages/capacitor-plugin/ios/Sources/GeolocationPlugin/GeolocationError.swift
Show resolved
Hide resolved
packages/capacitor-plugin/ios/Sources/GeolocationPlugin/GeolocationPlugin.swift
Outdated
Show resolved
Hide resolved
packages/capacitor-plugin/ios/Sources/GeolocationPlugin/GeolocationPlugin.swift
Outdated
Show resolved
Hide resolved
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.
Note: CI is failing on example app build for Android, but I think it could be due to the gradle version being updated on Capacitor Framework. Since this PR doesn't touch Android, I don't think we need to concern ourselves with it here. We'll probably be opening PRs soon to address that. Just wanted to let you know.
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.
Thanks
packages/capacitor-plugin/ios/Sources/GeolocationPlugin/GeolocationPlugin.swift
Show resolved
Hide resolved
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.
When testing I'm observing this behavior in the example app in this repo:
- I receive a timeout in
watchPosition(right now by setting a timeout of 1ms) - I try a get current position with timeout of 1ms. I receive two timeout errors - I believe 1 for this request and one for watch.
- I increase the timeout and call get current position. I start receiving locations in the watch.
All this leads me to believe that perhaps there needs to be logic in place to remove the watch callbacks in case of timeout error? Or to generalize the question: Which GeolocationError's are recoverable for a watch and which are not? I think the code assumes right now that all are, and that's not necessarily the case?
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.
Hm, let me take look.
I'll try to replicate it and see if the issue is related to the watch callbacks.
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.
With your change now, I'm having this issue: I have a watch in progress, and I trigger a second one (or a get current position) that times out, it also stops receiving updates for the first one.
This and the previous issue are all tied to the fact that timeouts are not specific to one single watch or location request.
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.
I’ve have figured out the cause in onLocationPermissionGranted()
let shouldRequestCurrentPosition = callbackManager?.locationCallbacks.isEmpty == false
let shouldRequestLocationMonitoring = callbackManager?.watchCallbacks.isEmpty == false
When a watch is already active and a getCurrentPosition is triggered, both flags are true. The current position itself doesn’t time out because we return the last known location from the watch.
However, startMonitoringLocation is also called again due to shouldRequestLocationMonitoring being true, which starts a new timer.
This issue existed before the current changes, so it will not be addressed in this PR.
| :path: "../../../../node_modules/.pnpm/@[email protected].3_@[email protected].3/node_modules/@capacitor/ios" | ||
| CapacitorGeolocation: | ||
| :path: "../../../capacitor-plugin" | ||
| :path: "../../../../node_modules/.pnpm/@capacitor+geolocation@file+packages+capacitor-plugin_@[email protected]/node_modules/@capacitor/geolocation" |
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.
I think due to something in pnpm or npm lock files, the example app now instead of depending on the local version, seems to depend on the latest published alpha - This causes npx cap sync to not update the geolocation code Pod for the example app.
I "fixed" on my end it by removing all lock files and node-modules, and sticking to just plain npm install on both plugin and example app.
We should strive for consistency in our repos, use pnpm for all, or npm for all, or at least use one of them for the same repository, not both.
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.
This doesn't really need to be addressed in this PR, and only affects the example app, so I'll be approving the PR, no point in dilly-dallying any further xD
# [8.0.0-next.4](v8.0.0-next.3...v8.0.0-next.4) (2025-11-04) ### Bug Fixes * **ios:** added timeout implementation for both getCurrentPosition and watchPosition ([#55](#55)) ([4c22ac3](4c22ac3))
Description
This PR adds a new IONGeolocationLib library (version 2.0.0) that includes logic to handle timeouts for both getCurrentPosition and watchPosition.
Context
Type of changes
Platforms affected
Tests
Test wit Location Sample in ODC
Screenshots (if appropriate)
Checklist
RNMT-XXXX <title>