Fence 2373 local radar sdk targeting#393
Fence 2373 local radar sdk targeting#393cameron-morrow-toptal wants to merge 37 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for targeting local iOS and Android native Radar SDKs during development by introducing two new configuration parameters (iosLocalRadarSdkPath and androidLocalRadarSdkPath) to the plugin configuration.
- Added local SDK path parameters to the RadarPluginProps interface
- Implemented iOS local SDK targeting by modifying Podfile and podspec files
- Implemented Android local SDK targeting by copying local AAR files and updating build.gradle
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/src/types.ts | Added optional iosLocalRadarSdkPath and androidLocalRadarSdkPath properties to RadarPluginProps interface |
| plugin/src/withRadarIOS.ts | Added logic to handle local iOS SDK path by updating podspec dependencies and Podfile configurations |
| plugin/src/withRadarAndroid.ts | Added logic to handle local Android SDK path by copying AAR files and updating build.gradle dependencies |
| example/app.json | Added example configuration for the new local SDK path parameters |
| example/App.tsx | Updated test API key and improved log message formatting |
| README.md | Added documentation explaining how to use the new local SDK targeting feature |
|
|
||
| if (!!args.iosLocalRadarSdkPath) { | ||
| const localPodspecContents = await fs.readFile(path.join(args.iosLocalRadarSdkPath, "RadarSDK.podspec"), "utf-8"); | ||
| const localRadarSdkVersion = localPodspecContents.match(/s\.version\s*=\s*["'](.+?)["']/)[1]; |
There was a problem hiding this comment.
The regex match could return null if the pattern doesn't match, which would cause a runtime error when accessing index [1]. Add null checking before accessing the match result.
| const localRadarSdkVersion = localPodspecContents.match(/s\.version\s*=\s*["'](.+?)["']/)[1]; | |
| const versionMatch = localPodspecContents.match(/s\.version\s*=\s*["'](.+?)["']/); | |
| if (!versionMatch) { | |
| throw new Error("Failed to extract RadarSDK version from the podspec file. Ensure the file contains a valid 's.version' declaration."); | |
| } | |
| const localRadarSdkVersion = versionMatch[1]; |
| "sdk-debug.aar" | ||
| ); | ||
|
|
||
| if (!fs.existsSync(aarPath)) { |
There was a problem hiding this comment.
[nitpick] Using synchronous fs.existsSync in an async function is inconsistent with the async pattern. Consider using fs.promises.access() or await fs.stat() to maintain async consistency.
| if (!fs.existsSync(aarPath)) { | |
| try { | |
| await fs.promises.access(aarPath); | |
| } catch (err) { |
| ); | ||
| } | ||
|
|
||
| fs.copyFileSync( |
There was a problem hiding this comment.
[nitpick] Using synchronous fs.copyFileSync in an async function is inconsistent with the async pattern used elsewhere. Consider using fs.promises.copyFile() for consistency.
| fs.copyFileSync( | |
| await fs.copyFile( |
| ) | ||
| ); | ||
|
|
||
| fs.copyFileSync( |
There was a problem hiding this comment.
[nitpick] Using synchronous fs.copyFileSync in an async function is inconsistent with the async pattern used elsewhere. Consider using fs.promises.copyFile() for consistency.
| fs.copyFileSync( | |
| await fs.copyFile( |
| if (fs.existsSync(buildGradlePath)) { | ||
| const buildGradleContents = fs.readFileSync(buildGradlePath, "utf8"); | ||
| const updatedBuildGradleContents = buildGradleContents.replace( | ||
| /api 'io\.radar:sdk:.+/, | ||
| "implementation files('local_radar_sdk.aar')" | ||
| ); | ||
| fs.writeFileSync(buildGradlePath, updatedBuildGradleContents); |
There was a problem hiding this comment.
[nitpick] Using synchronous fs.existsSync in an async function is inconsistent with the async pattern. Consider using fs.promises.access() or await fs.stat() to maintain async consistency.
| if (fs.existsSync(buildGradlePath)) { | |
| const buildGradleContents = fs.readFileSync(buildGradlePath, "utf8"); | |
| const updatedBuildGradleContents = buildGradleContents.replace( | |
| /api 'io\.radar:sdk:.+/, | |
| "implementation files('local_radar_sdk.aar')" | |
| ); | |
| fs.writeFileSync(buildGradlePath, updatedBuildGradleContents); | |
| try { | |
| await fs.promises.access(buildGradlePath); | |
| const buildGradleContents = fs.readFileSync(buildGradlePath, "utf8"); | |
| const updatedBuildGradleContents = buildGradleContents.replace( | |
| /api 'io\.radar:sdk:.+/, | |
| "implementation files('local_radar_sdk.aar')" | |
| ); | |
| fs.writeFileSync(buildGradlePath, updatedBuildGradleContents); | |
| } catch { | |
| // File does not exist, no action needed |
| ); | ||
|
|
||
| if (fs.existsSync(buildGradlePath)) { | ||
| const buildGradleContents = fs.readFileSync(buildGradlePath, "utf8"); |
There was a problem hiding this comment.
[nitpick] Using synchronous fs.readFileSync in an async function is inconsistent with the async pattern used elsewhere. Consider using fs.promises.readFile() for consistency.
| const buildGradleContents = fs.readFileSync(buildGradlePath, "utf8"); | |
| const buildGradleContents = await fs.readFile(buildGradlePath, "utf8"); |
| /api 'io\.radar:sdk:.+/, | ||
| "implementation files('local_radar_sdk.aar')" | ||
| ); | ||
| fs.writeFileSync(buildGradlePath, updatedBuildGradleContents); |
There was a problem hiding this comment.
[nitpick] Using synchronous fs.writeFileSync in an async function is inconsistent with the async pattern used elsewhere. Consider using fs.promises.writeFile() for consistency.
| const updatedPodfileContents = podfileContents.replace( | ||
| /(target '(\w+)' do)/g, | ||
| `$1\n pod 'RadarSDK', :path => '${path.resolve(args.iosLocalRadarSdkPath)}'` | ||
| ); | ||
| await fs.writeFile(podfilePath, updatedPodfileContents); |
There was a problem hiding this comment.
The regex replacement could potentially add multiple pod entries if the target pattern appears multiple times in the Podfile. Consider checking if the local pod entry already exists before adding it.
| const updatedPodfileContents = podfileContents.replace( | |
| /(target '(\w+)' do)/g, | |
| `$1\n pod 'RadarSDK', :path => '${path.resolve(args.iosLocalRadarSdkPath)}'` | |
| ); | |
| await fs.writeFile(podfilePath, updatedPodfileContents); | |
| // Check if the RadarSDK pod entry already exists | |
| if (!podfileContents.includes(`pod 'RadarSDK', :path => '${path.resolve(args.iosLocalRadarSdkPath)}'`)) { | |
| const updatedPodfileContents = podfileContents.replace( | |
| /(target '(\w+)' do)/g, | |
| `$1\n pod 'RadarSDK', :path => '${path.resolve(args.iosLocalRadarSdkPath)}'` | |
| ); | |
| await fs.writeFile(podfilePath, updatedPodfileContents); | |
| } |
| - build native app using expo pre-build and `react-native-plugin` with `npm run install-radar-rebuild`. | ||
| - run iOS and android example app with `npx expo run:ios` or `npx expo run:android`. | ||
|
|
||
| To run example app with local `RadarSDK` native dependencies: |
|
|
||
| MapLibreGL.setAccessToken(null); | ||
|
|
||
| Radar.initialize("prj_test_pk_b2e957d3287bed449edede86ed2006a9c93f7f51", true); |
There was a problem hiding this comment.
might want to rotate that key
There was a problem hiding this comment.
already did 😅
| androidActivityRecognition?: boolean; | ||
| addRadarSDKMotion?: boolean; | ||
| iosNSMotionUsageDescription?: string; | ||
| iosLocalRadarSdkPath?: string; |
There was a problem hiding this comment.
I really do appreciate the initiative you took creating a plugin to make local builds for testing easier.
I also have no doubt that this would make the dev process easier.
The only reservation I have is about this being "public" facing. Context is that expo plugins are sometime fragile and we don't want us to be ever on the hook for fixing this for extending compatibility for this plugin.
I think the combination of
- not referencing them in the developer docs
- adding comments in code that these are for testing only
Should allow us to ship this while de-risking. Any thoughts @ShiCheng-Lu @lmeier ?
There was a problem hiding this comment.
makes sense. I just took inspiration from the waypoint install scripts, (e.g. https://github.com/radarlabs/waypoint/blob/main/custom-install.sh)
added two params to
app.jsonto make it much easier to target local iOS and android native sdks during development