fix(ios): added timeout implementation for both getCurrentPosition and watchPosition #17
Conversation
OS-pedrogustavobilro
left a comment
OS-pedrogustavobilro
left a comment
There was a problem hiding this comment.
Haven't reviewed the whole PR yet, just wanted to leave this first comment for a potential issue that needs further discussion.
|
|
||
| /** | ||
| * Returns a timeout failure, closed over a specified timeout value and error callback. | ||
| * @param onError the error callback | ||
| * @param timeout timeout in ms | ||
| * @param isWatch returns `true` if the caller of this function was the from the watch flow | ||
| * @param id the watch ID | ||
| * @returns the timeout's ID | ||
| */ | ||
| #createTimeout(onError: (error: PluginError) => void, timeout: number | undefined, isWatch: boolean, id: string): ReturnType<typeof setTimeout> { | ||
|
|
||
| let t = setTimeout(() => { | ||
| if (isWatch === true) { | ||
| this.clearWatch({ id }) | ||
| } | ||
| onError({ | ||
| code: "OS-PLUG-GLOC-0010", | ||
| message: "Could not obtain location in time. Try with a higher timeout." | ||
| }) | ||
| }, timeout) | ||
| return t | ||
| } | ||
|
|
There was a problem hiding this comment.
By removing this timeout entirely from the outsystems wrapper, we end up impacting end users on mobile apps that have an older version of the plugin but get this code updated via OTA update.
The impact of this is that instead of timing out, location requests can get stuck, and potentially client apps get stuck as well.
You can test this by doing the following: Point the cordova plugin to the latest stable version in Extensibility Configuration, and update the wrapper script in the OutSystems plugin to the one from that latest stable version. Publish the OutSystems Plugin, and make the sample app use that version of the Plugin. Generate a mobile app and install it. Now update the wrapper and cordova plugin branch to the one from this PR. Update the sample app. A new mobile app will be generated, but use the current one. You'll never be able to trigger a timeout in iOS.
Whether this is a big problem or not - I'm not sure - but it is a kind of situation we have tried to avoid, and depending on your definition of "breaking change", it could be a "breaking change". Android should be ok because (for the most part at least), it should already handle timeouts natively, and for me usually the CLLocation replies quick enough on iOS to never trigger a timeout. But that's from our tests, in our devices and specific conditions. Once this would get out to the wild, it could affect customers, and it wouldn't exactly be an easy issue to fix after it is in the wild I think.
To fix this, I'm not sure if there's a good solution. I don't think we can check clearly at runtime what version of a plugin is being used, in order to have backwards-compatible behavior.
What I'm thinking have a method on the Cordova and Capacitor for like hasNativeTimeoutHandling, which is ok for Cordova, but having this on Capacitor is kind funky, since it would have relevance only for low-code developers.
But anyway, the discussion is two-fold:
- Is the issue of lack of timeout for existing mobile app versions relevant?
- If it is relevant, how do we address it?
There was a problem hiding this comment.
Yeah, that's a good point. Probably the easiest fix is to have a method like hasNativeTimeoutHandling, but I'm not sure if it's the best solution, and I don't have a better one 🤷♂️.
Anyway, I will test it and see how it works, I mean test the flow you described.
There was a problem hiding this comment.
@OS-pedrogustavobilro
UPDATE:
I tested it, and the timeout doesn’t trigger anymore. In my case, it wasn’t really a problem because the location came back quite fast, but of course, this may vary depending on the user’s conditions and device.
There was a problem hiding this comment.
Fair enough, so yeah I think this should be discussed amongst the team.
@alexgerardojacinto if you'd like to chime in.
There was a problem hiding this comment.
My two cents:
I understand that in our tests (mine included) the location is returned pretty quickly and so we don't really notice the lack of a timeout. However, in real-usage scenarios (e.g. delivery guy using his company app on a somewhat remote location) location retrieval can take a long time and that's what the timeout is there for - to avoid the app being stuck for a long time waiting for the location.
If we can avoid adding this potencial issue in our customer's apps, we should. As a last resort, if we couldn't, we could always say this version has a breaking change and make it clear in the release notes that a new build should be generated to use this new version of the plugin.
But if we can avoid it, we should.
@OS-ruimoreiramendes @OS-pedrogustavobilro let me know what you think.
There was a problem hiding this comment.
I thought we couldn't get the plugin version at runtime, but if this command works, I think this can be a good approach. At least it’s more generic.
There was a problem hiding this comment.
I ended up implementing the hasNativeTimeoutHandling method.
I initially tried the approach suggested by André, using cordova.require('cordova/plugin_list').metadata["com.outsystems.plugins.geolocation"] to detect the plugin version at runtime, but this caused the app to crash.
The plugin metadata only becomes available after deviceready, and since this check needs to run inside the wrapper (before deviceready fires), the call was happening too early, leading to runtime errors.
To avoid this timing dependency and ensure consistent behavior, I followed Pedro’s approach instead, exposing a method that explicitly reports whether the plugin supports native timeout handling.
This keeps the logic reliable and compatible across versions.
There was a problem hiding this comment.
I think I didn’t express my idea clearly.
The idea behind using cordova.require... is to add it directly to the wrapper, and depending on the returned value, either call the existing timeout logic or call the plugin with the timeout parameter.
Regarding getVersion, that’s a separate idea, the goal here is to create a function that returns the version number directly from plugin.xml, if possible, or from a string that could be updated by a hook.
However, I’m also fine with the hasNativeTimeoutHandling approach, so please go ahead. 👍
There was a problem hiding this comment.
Thanks for the clarification.
There was a problem hiding this comment.
So in sum: hasNativeTimeoutHandling was added to the cordova plugin in this PR. The OutSystems wrapper will compare older to newer Cordova Plugin versions by the existence of this new method and if it returns true; if false, will still implement a timeout in wrapper side, thus maintaining backwards-compatibility.
For capacitor, we did not add hasNativeTimeoutHandling, assuming always the default value of true (Capacitor in OutSystems is currently in beta and the capacitor version of this plugin was checked not be in use in production apps, so there is no true "breaking change" here).
OS-ruimoreiramendes
changed the title
OS-ruimoreiramendes
requested review from
OS-pedrogustavobilro,
alexgerardojacinto and
andredestro
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 or O11
Screenshots (if appropriate)
Checklist