Skip to content

[MOO-2225] migrate geolocation to react native nitro geolocation - #552

Open
stelselim wants to merge 7 commits into
mainfrom
moo-2225/migrate-new-geo-location-library
Open

[MOO-2225] migrate geolocation to react native nitro geolocation#552
stelselim wants to merge 7 commits into
mainfrom
moo-2225/migrate-new-geo-location-library

Conversation

@stelselim

@stelselim stelselim commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Migrate react native nitro geolocation

@stelselim
stelselim requested a review from a team as a code owner July 20, 2026 15:26
@stelselim stelselim changed the title feat: migrate to new geolocation library and update related functiona… [MOO-2225] migrate geolocation to react native nitro geolocation Jul 22, 2026
import type { Platform, NativeModules } from "react-native";
import type { GeoError, GeoPosition, GeoOptions } from "../../typings/Geolocation";
import { Platform } from "react-native";
import { getCurrentPosition, GeolocationResponse, LocationRequestOptions } from "react-native-nitro-geolocation";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good news — I think we may be able to drop the hand-rolled web branch entirely here. The library ships web support for the Modern API root import: browser builds resolve to src/index.web.tsx, which is backed by navigator.geolocation and doesn't load any Nitro native bindings. Its getCurrentPosition has the same signature and also returns a promise, so await getCurrentPosition(options) should work unchanged on both platforms and the whole isReactNative/isWeb split could go away. Same applies to watchPosition/unwatch in GetCurrentLocationMinimumAccuracy.ts — the web entry exports those with identical signatures (string tokens included).

One thing worth verifying before we rely on it: whether the Mendix web client resolves the package's browser/exports condition. If it does, this simplifies both actions a lot. If it doesn't, then the manual branch is the right call and we should keep it with a short comment saying why, so the reason is obvious next time someone reads it.


import type { Platform, NativeModules } from "react-native";
import type { GeoError, GeoPosition, GeoOptions } from "../../typings/Geolocation";
import { Platform } from "react-native";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This import should probably happen inside the native branch instead, via require("react-native") — that's the pattern the other cross-platform actions here use (OpenURL, Share, CallPhoneNumber). Top-level ESM imports are eager, so on web this still resolves even though only the iOS timeout clamp actually needs it.

Platform is used in just the one place (the Platform.OS === "ios" check in buildLocationOptions), so making it lazy is cheap:

if (isReactNative && require("react-native").Platform.OS === "ios") { ... }

Same applies to GetCurrentLocationMinimumAccuracy.ts:9.

const options = buildLocationOptions(timeout, maximumAge, highAccuracy);
let lastAccruedPosition: GeolocationResponse | undefined;

const timeoutMs = timeout ? timeout.toNumber() : 30000;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Small correctness issue here: new Big(0) is truthy, so a configured timeout of 0 gives timeoutMs = 0, and setTimeout then fires onTimeout on the next tick — rejecting with "Timeout expired" before any position can arrive.

It's also worth noting that buildLocationOptions below maps 0 to 3600000 ("no timeout"), so with timeout = 0 the location layer is told one hour while this watchdog fires immediately — the two timeout computations end up disagreeing. Could we derive both from a single clamped value? Something like:

const timeoutMs = options.timeout ?? 30000;

One related thing: the 0 -> 3600000 clamp is currently inside the Platform.OS === "ios" check, so 0 still reaches Android and navigator.geolocation unclamped, and on web timeout: 0 means "fail immediately with TIMEOUT" on every update. Might be worth lifting that clamp out of the iOS-only branch.

The : 30000 fallback itself is a nice improvement over what was there before, where an empty timeout gave Number(undefined) -> NaN.

pos => resolve(normalizeWebPosition(pos)),
err => reject(err),
{
timeout: options.timeout ?? undefined,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Minor: the ?? undefined is redundant here — options.timeout and options.maximumAge are already typed number | undefined, so this is a no-op and can just be options.timeout / options.maximumAge.

Same two lines exist in GetCurrentLocationMinimumAccuracy.ts:71-72.

}
});
} catch (error: any) {
const message = error instanceof Error ? error.message : error?.message ?? String(error);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This narrowing can be simplified — the right-hand side already handles the Error case, since error.message works either way. So this is equivalent:

const message = error?.message ?? String(error);

timeoutNumber = 3600000;
}
}
function normalizeWebPosition(pos: GeolocationPosition): GeolocationResponse {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

normalizeWebPosition is duplicated verbatim in both geolocation actions, and the library already exports an equivalent (normalizePosition in its web entry, which also sets provider).

If we end up using the library's built-in web support, this can be removed outright. If we keep the manual path, could we add a short note that the two copies need to stay in sync? Sharing across two action files isn't really possible in the Mendix model as far as I know, so a comment is probably the most we can do.

* On hybrid and native platforms the permission should be requested with the `RequestLocationPermission` action.
*
* For good user experience, disable the nanoflow during action using property `Disabled during action` if youre using `Call a nanoflow button` to run JS Action `Get current location with minimum accuracy`.
* For good user experience, disable the nanoflow during action using property `Disabled during action` if you're using `Call a nanoflow button` to run JS Action `Get current location with minimum accuracy`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This quote change (the apostrophe in "you're") is outside the BEGIN USER CODE block, so Studio Pro will regenerate over it on the next deploy. Probably worth reverting just to keep the diff focused on the migration.

"dependencies": {
"@react-native-async-storage/async-storage": "2.2.0",
"@react-native-community/geolocation": "3.4.0",
"invariant": "^2.2.4",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

invariant looks like it can go now — it was only here for @react-native-community/geolocation. The comment in configs/jsactions/rollup.config.mjs (the nanoflowcommons branch) says as much, and it links to the exact line in that library that used it silently.

I checked the remaining nanoflowcommons dependencies (async-storage, js-base64, permissions, geocoder, nitro-geolocation, nitro-modules) and none of them reference invariant, so both this entry and the copy step in rollup.config.mjs could be removed. That would be a nice bit of cleanup to land with the migration, since this PR is what makes it dead.

"name": "nanoflow-actions-native",
"moduleName": "Nanoflow Commons",
"version": "7.1.0",
"version": "7.2.0",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you double-check this version bump? CHANGELOG.md already has a ## [7.2.0] Nanoflow Commons - 2026-7-3 section, and it describes the OpenURL offline-db change rather than this migration — so this would be a second, different 7.2.0.

I had a look at the release scripts to make sure this wasn't handled automatically: BumpVersion.ts only scans packages/pluggableWidgets, so it doesn't touch this package, and marketplaceRelease.js reads the version straight out of package.json to publish under. So it looks like whatever is here is what ships. 7.3.0 plus a fresh changelog section for the migration is probably what we want — happy to be corrected if the release flow works differently than I'm reading it.

compilerOptions: {
newLine: "CRLF"
newLine: "CRLF",
jsx: "react-native"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Worth documenting why this is needed, since it's non-obvious. react-native-nitro-geolocation ships no compiled JS at all — the tarball has 0 .js files and 54 .ts/.tsx, with main: "src/index" and browser: "src/index.web.tsx" — so the TS plugin needs jsx set in order to parse the .tsx entry point. I confirmed it's load-bearing: without it the build fails with TS6142: Module ... was resolved to '.../src/index.tsx', but '--jsx' is not set.

The flag itself is fine. The thing I'd want confirmed before merge is the consequence: the client now resolves raw TSX at runtime for this dependency, where the old library shipped lib/commonjs + lib/module. That's worth a quick sanity check in a real app (web especially), independent of the web-support question on GetCurrentLocation.ts.

Could you add a short comment here noting why jsx is required? Otherwise it looks unrelated to a geolocation migration and someone may remove it later.

@vadymv-mendix

vadymv-mendix commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Nice cleanup overall — dropping the RNFusedLocation/RNCGeolocation NativeModules probing and the hand-written typings/Geolocation.d.ts shim removes real complexity, and that shim only existed to paper over the two competing implementations in the old library. Adding clearTimeout/clearWatch to onError in the minimum-accuracy action also fixes a leak that was there before, which is a good catch.

I've left a few inline comments — the only one I'd call blocking is the timeout = 0 handling in GetCurrentLocationMinimumAccuracy.ts, since new Big(0) is truthy and the watchdog ends up firing immediately.

Two things that would be good to add to the PR:

  • A CHANGELOG.md entry under [Unreleased].
  • Removal of the now-unused invariant dependency and its copy step in configs/jsactions/rollup.config.mjs (details in the inline comment).

And one non-blocking design question, mostly for your judgement since you know the intended UX better than I do: onError now rejects even when lastAccruedPosition already holds a usable fix. For an action whose whole purpose is "best effort within a timeout," falling back to the best-so-far on a transient POSITION_UNAVAILABLE might serve users better than failing outright. This behaves the same as before the PR so it's not a regression — just seemed worth a deliberate decision rather than inheriting it.

Last thing, and the main risk I see: the new library ships no compiled JavaScript, so the client resolves raw .tsx at runtime (hence the new jsx flag in the rollup config). Combined with the web-support question on GetCurrentLocation.ts:10, I'd want this properly tested in a real app on both native and web before merging — that's the one thing I couldn't verify from the repo alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants