fix(ios): Add OnsideKit as a default dependency in ExpoIap.podspec#321
fix(ios): Add OnsideKit as a default dependency in ExpoIap.podspec#321retweakr wants to merge 6 commits intohyochan:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a dependency issue by explicitly adding Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds an iOS Onside subspec to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Plugin as Config Plugin
participant Podfile as Podfile (file)
participant Podspecs as CocoaPods/Podspecs
participant WA as WarningAggregator
Plugin->>Podfile: read Podfile content
Plugin->>Plugin: ensureOnsidePodIOS(content)
alt Onside subspec or OnsideKit missing
Plugin->>Podspecs: check for existing `OnsideKit` / `ExpoIap/Onside`
Podspecs-->>Plugin: discovery result
Plugin->>Podfile: insert `pod 'ExpoIap/Onside', :path => EXPO_IAP_IOS_PATH` (and/or OnsideKit pod line)
Podfile-->>Plugin: updated content
Plugin->>WA: addWarningIOS(target-block insertion warning)
else Already present
Plugin-->>Podfile: no-op (idempotent)
end
Plugin->>Podfile: write updated Podfile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes OnsideKit a direct dependency in the podspec. However, this creates a conflict with the existing config plugin logic in plugin/src/withIAP.ts, which still tries to inject the same dependency into the user's Podfile. This redundancy can lead to build failures and versioning issues. I recommend that the logic in the config plugin be removed if this change is to proceed. Furthermore, making this a mandatory dependency for all users might not be ideal, as it could increase app size for those not using the Onside feature.
ios/ExpoIap.podspec
Outdated
| s.dependency 'openiap', "#{versions['apple']}" | ||
|
|
||
| # OnsideKit is optional; added via ensureOnsidePod() in Podfile when modules.onside is enabled | ||
| s.dependency 'OnsideKit' |
There was a problem hiding this comment.
Making OnsideKit a direct dependency here conflicts with the existing logic in plugin/src/withIAP.ts. The ensureOnsidePod function in that file still injects OnsideKit into the user's Podfile when modules.onside is enabled. This can cause two problems:
- A potential CocoaPods conflict if
OnsideKitis defined both here and in the mainPodfile. - The plugin injects a specific version from a URL (
:podspec => '...'), while this dependency is unversioned, which could lead to version mismatches.
If the goal is to make OnsideKit a direct dependency, the corresponding injection logic in withIAP.ts should be removed in the same pull request to avoid breaking user builds. Since that file isn't modified here, I suggest reverting this change and creating a new PR that addresses both files together.
# OnsideKit is optional; added via ensureOnsidePod() in Podfile when modules.onside is enabled
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
=======================================
Coverage 92.05% 92.05%
=======================================
Files 7 7
Lines 642 642
Branches 202 202
=======================================
Hits 591 591
Misses 47 47
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Thanks for catching this, Artyom! I appreciate you staying up to work through it. You're right that the current However, adding Proposed approach: subspecInstead, we can use an optional subspec: s.subspec 'Onside' do |ss|
ss.dependency 'OnsideKit'
endThe Swift source files already use Release planSince this doesn't cause side effects for non-Onside users (the |
retweakr
force-pushed
the
fix/onside-kit-podspec
branch
2 times, most recently
from
c2518bb to
34c8e6a
Compare
…ate Podfile handling
retweakr
force-pushed
the
fix/onside-kit-podspec
branch
from
bdbdbf4 to
6b49a29
Compare
hyochan
left a comment
hyochan
left a comment
There was a problem hiding this comment.
Thanks for the subspec approach, Artyom! The podspec changes look good — s.subspec 'Onside' with s.default_subspecs = [] is exactly the right pattern.
However, there's an issue in the plugin logic. In withIAP.ts:
// 3) Always add OnsideKit; add ExpoIap/Onside only when onside is enabled
content = ensureOnsidePod(content, options?.enableOnside ?? false);Inside ensureOnsidePod, pod 'OnsideKit' is added unconditionally — even when onside: false. This means all users (including those who don't use Onside) will have OnsideKit installed in their Podfile, which is what we want to avoid.
Both lines should only be added when onside: true:
pod 'OnsideKit', :podspec => '...'— needed so CocoaPods knows the source URLpod 'ExpoIap/Onside', :path => '...'— needed so ExpoIap can see OnsideKit
When onside: false, neither should be added. The call site should stay conditional:
if (options?.enableOnside) {
content = ensureOnsidePod(content);
}Or keep the boolean parameter but guard both pod lines behind it inside the function.
The podspec side is good to go 👍
hyochan
left a comment
hyochan
left a comment
There was a problem hiding this comment.
The podspec subspec approach is correct (s.subspec 'Onside' + s.default_subspecs = []), but the plugin logic needs a fix.
In ensureOnsidePod, pod 'OnsideKit' is added unconditionally regardless of the onside flag. This causes all users to install OnsideKit even when they don't use Onside.
Both pod lines should only be added when onside: true:
pod 'OnsideKit', :podspec => '...'— for CocoaPods source resolutionpod 'ExpoIap/Onside', :path => '...'— so ExpoIap can import OnsideKit
See my previous comment for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/src/withIAP.ts (1)
271-271: Rename iOS-only helper withIOSsuffix.
ensureOnsidePodis iOS-specific Podfile logic; considerensureOnsidePodIOSto align naming with repository conventions.As per coding guidelines
**/*.{ts,tsx,js,jsx}: Functions that only operate on one platform must carry the suffix:nameIOSornameAndroid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/src/withIAP.ts` at line 271, The function ensureOnsidePod is iOS-specific and should be renamed to ensureOnsidePodIOS to follow the platform-suffix convention; update the function declaration name and all references/usages (imports, exports, calls) to ensureOnsidePodIOS and adjust any TypeScript types or JSDoc that reference the old name so compilation and tooling continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/src/withIAP.ts`:
- Around line 277-283: The current code unconditionally injects pod 'OnsideKit'
regardless of the onside flag and uses the function name ensureOnsidePod which
must be platform-specific; modify the pod injection block so pod 'OnsideKit' is
only added when onside is true (use the existing onside boolean and
alreadyHasOnsideKit guard) and rename the function ensureOnsidePod to
ensureOnsidePodIOS (update its definition and all call sites, e.g., where
ensureOnsidePod is invoked) so the iOS-specific helper follows the naming
convention; ensure the podLines concatenation for OnsideKit and ExpoIap/Onside
remains correct under the new gating and that references to alreadyHasOnsideKit
and alreadyHasExpoIapOnside are preserved.
---
Nitpick comments:
In `@plugin/src/withIAP.ts`:
- Line 271: The function ensureOnsidePod is iOS-specific and should be renamed
to ensureOnsidePodIOS to follow the platform-suffix convention; update the
function declaration name and all references/usages (imports, exports, calls) to
ensureOnsidePodIOS and adjust any TypeScript types or JSDoc that reference the
old name so compilation and tooling continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 304933d7-3dc0-4d87-8965-1fb472746081
📒 Files selected for processing (2)
ios/ExpoIap.podspecplugin/src/withIAP.ts
- Add ExpoIap/Onside subspec with default_subspecs = [] so OnsideKit is only included when explicitly enabled - Rename ensureOnsidePod to ensureOnsidePodIOS per naming convention - Keep call site conditional: only called when enableOnside is true - Add ensureOnsidePodIOS tests covering onside true/false scenarios Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@retweakr I pushed a commit on top of your branch with the fix. Here's what changed: Key changes:
Could you test this with your setup ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/__tests__/withIAP.test.ts (1)
247-251: Consider asserting the warning call for completeness.The test correctly verifies content is unchanged, but could be strengthened by asserting that
WarningAggregator.addWarningIOSwas called when no target block is found.💡 Optional enhancement
+import {WarningAggregator} from 'expo/config-plugins'; + it('returns unchanged content when no target block found', () => { const noPodfile = '# empty'; const result = ensureOnsidePodIOS(noPodfile); expect(result).toBe(noPodfile); + expect(WarningAggregator.addWarningIOS).toHaveBeenCalledWith( + 'expo-iap', + expect.stringContaining('Could not find a target block'), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/__tests__/withIAP.test.ts` around lines 247 - 251, The test for ensureOnsidePodIOS should also assert that WarningAggregator.addWarningIOS was invoked when no target block exists; update the test case (the it('returns unchanged content when no target block found') block) to mock or spy on WarningAggregator.addWarningIOS and add an expectation that it was called (e.g., toHaveBeenCalled or toHaveBeenCalledWith appropriate args) after calling ensureOnsidePodIOS(noPodfile), while keeping the existing expect(result).toBe(noPodfile).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin/__tests__/withIAP.test.ts`:
- Around line 247-251: The test for ensureOnsidePodIOS should also assert that
WarningAggregator.addWarningIOS was invoked when no target block exists; update
the test case (the it('returns unchanged content when no target block found')
block) to mock or spy on WarningAggregator.addWarningIOS and add an expectation
that it was called (e.g., toHaveBeenCalled or toHaveBeenCalledWith appropriate
args) after calling ensureOnsidePodIOS(noPodfile), while keeping the existing
expect(result).toBe(noPodfile).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63de7f5b-47ce-4bea-901e-57bfef1ce46a
📒 Files selected for processing (3)
ios/ExpoIap.podspecplugin/__tests__/withIAP.test.tsplugin/src/withIAP.ts
|
@retweakr Pushed a new approach — the subspec doesn't work because OnsideKit isn't on CocoaPods CDN yet (confirmed by Alex). New approach: Instead of a subspec dependency, the plugin now:
This makes Once OnsideKit is published to CocoaPods CDN (Alex mentioned at 1.0), we can simplify this to a subspec. Could you test both cases?
Feel free to revert or modify if needed 🙏 |
…ubspec OnsideKit is not on CocoaPods CDN yet, so subspec dependency fails to resolve. Instead, inject a post_install hook that adds OnsideKit's build dir to ExpoIap's SWIFT_INCLUDE_PATHS, making #if canImport(OnsideKit) work without requiring a podspec dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hyochan
force-pushed
the
fix/onside-kit-podspec
branch
from
17b20d1 to
15fa2b5
Compare
Replace post_install hook workaround with a clean subspec dependency. The plugin adds `pod 'ExpoIap/Onside'` to Podfile only when onside is enabled. Requires OnsideKit to be published on CocoaPods CDN. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@retweakr Simplified the approach — removed the Now it's a clean subspec approach: Podspec: s.subspec 'Onside' do |ss|
ss.dependency 'OnsideKit'
end
s.default_subspecs = []Plugin: When pod 'ExpoIap/Onside', :path => '../node_modules/expo-iap/ios'This requires OnsideKit to be on CocoaPods CDN — which is why I asked about publishing it. Once Alex publishes it (even a pre-release), this should just work. Could you check with Alex on the CDN publishing timeline? That's the blocker — once it's there, this PR is ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/src/withIAP.ts`:
- Around line 267-287: ensureOnsidePodIOS currently injects "pod
'ExpoIap/Onside'" unconditionally which breaks pod install because the OnsideKit
dependency isn't on the public CocoaPods CDN; update ensureOnsidePodIOS to
either (A) inject the required CocoaPods source/repo entry before inserting the
podLine (so the resolver can find OnsideKit) or (B) avoid adding the subspec and
instead emit a clear WarningAggregator.addWarningIOS message instructing the
user how to add the private source or enable Onside manually; locate the logic
in ensureOnsidePodIOS (look for EXPO_IAP_IOS_PATH, podLine, targetMatch) and
implement one of these fixes so pod install can resolve OnsideKit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c944b6a-470c-41e3-8645-97441158b101
📒 Files selected for processing (3)
ios/ExpoIap.podspecplugin/__tests__/withIAP.test.tsplugin/src/withIAP.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/tests/withIAP.test.ts
| export const ensureOnsidePodIOS = (content: string): string => { | ||
| if (/^\s*pod\s+['"]ExpoIap\/Onside['"].*$/m.test(content)) { | ||
| return content; | ||
| } | ||
|
|
||
| const targetMatch = content.match(/target\s+'[^']+'\s+do\s*\n/); | ||
| if (!targetMatch) { | ||
| WarningAggregator.addWarningIOS( | ||
| 'expo-iap', | ||
| 'Could not find a target block in Podfile when adding OnsideKit; skipping installation.', | ||
| 'Could not find a target block in Podfile when adding ExpoIap/Onside; skipping installation.', | ||
| ); | ||
| return content; | ||
| } | ||
|
|
||
| const podLine = ` pod 'ExpoIap/Onside', :path => '${EXPO_IAP_IOS_PATH}'\n`; | ||
| const insertIndex = targetMatch.index! + targetMatch[0].length; | ||
| const before = content.slice(0, insertIndex); | ||
| const after = content.slice(insertIndex); | ||
|
|
||
| logOnce('📦 expo-iap: Added OnsideKit pod to Podfile'); | ||
| logOnce('📦 expo-iap: Added ExpoIap/Onside subspec to Podfile'); | ||
|
|
||
| return `${before}${podLine}\n${after}`; | ||
| return content.slice(0, insertIndex) + podLine + content.slice(insertIndex); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking CocoaPods availability for OnsideKit..."
status=$(curl -s -o /tmp/onsidekit.html -w "%{http_code}" https://cocoapods.org/pods/OnsideKit || true)
echo "HTTP status (https://cocoapods.org/pods/OnsideKit): $status"
echo
echo "Checking current plugin pod injection logic..."
rg -n "ensureOnsidePodIOS|ExpoIap/Onside|OnsideKit" plugin/src/withIAP.ts
echo
echo "Expected verification outcome:"
echo "- If status is not 200 and plugin does not inject a direct OnsideKit podspec fallback,"
echo " Onside-enabled Pod install can fail on dependency resolution."Repository: hyochan/expo-iap
Length of output: 838
🏁 Script executed:
cat -n ios/ExpoIap.podspecRepository: hyochan/expo-iap
Length of output: 2087
🏁 Script executed:
rg -A 10 "subspec.*[Oo]nside" ios/ExpoIap.podspecRepository: hyochan/expo-iap
Length of output: 297
🏁 Script executed:
find . -name "*.podspec" -o -name "*.md" | xargs rg -l "OnsideKit|Onside" 2>/dev/null | head -20Repository: hyochan/expo-iap
Length of output: 362
🏁 Script executed:
head -100 docs/docs/guides/onside-integration.mdRepository: hyochan/expo-iap
Length of output: 3759
🏁 Script executed:
head -100 docs/blog/2026-03-15-3.4.10-onside-integration.mdRepository: hyochan/expo-iap
Length of output: 3131
🏁 Script executed:
rg -n "OnsideKit|podspec|source|private" plugin/src/withIAP.ts | head -30Repository: hyochan/expo-iap
Length of output: 330
🏁 Script executed:
rg -B 5 -A 5 "ensureOnsidePodIOS" plugin/src/withIAP.ts | head -50Repository: hyochan/expo-iap
Length of output: 653
🏁 Script executed:
rg -n "OnsideKit" README.md docs/Repository: hyochan/expo-iap
Length of output: 42
🏁 Script executed:
grep -r "OnsideKit" . --include="*.json" --include="*.ts" --include="*.tsx" 2>/dev/null | grep -v node_modulesRepository: hyochan/expo-iap
Length of output: 145
🏁 Script executed:
grep -r "OnsideKit\|onside-kit\|onside_kit" . --include="*.md" --include="*.json" 2>/dev/null | grep -v node_modules | grep -v ".next"Repository: hyochan/expo-iap
Length of output: 42
🏁 Script executed:
cat plugin/src/withIAP.ts | sed -n '475,510p'Repository: hyochan/expo-iap
Length of output: 1255
ExpoIap/Onside injection will break pod install for Onside-enabled users due to unresolvable OnsideKit dependency.
The Onside subspec (ios/ExpoIap.podspec:32) declares ss.dependency 'OnsideKit', but OnsideKit is not available on the public CocoaPods CDN (HTTP 404). When users enable Onside via the plugin, pod install fails during dependency resolution. The plugin must either inject a CocoaPods source for OnsideKit or implement an alternative dependency resolution strategy. Currently, documentation presents Onside as ready to use with "Easy Setup," but enabling it produces a broken build.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/src/withIAP.ts` around lines 267 - 287, ensureOnsidePodIOS currently
injects "pod 'ExpoIap/Onside'" unconditionally which breaks pod install because
the OnsideKit dependency isn't on the public CocoaPods CDN; update
ensureOnsidePodIOS to either (A) inject the required CocoaPods source/repo entry
before inserting the podLine (so the resolver can find OnsideKit) or (B) avoid
adding the subspec and instead emit a clear WarningAggregator.addWarningIOS
message instructing the user how to add the private source or enable Onside
manually; locate the logic in ensureOnsidePodIOS (look for EXPO_IAP_IOS_PATH,
podLine, targetMatch) and implement one of these fixes so pod install can
resolve OnsideKit.
There was a problem hiding this comment.
Acknowledged — this is a known blocker. OnsideKit is not on CocoaPods CDN yet (confirmed by the OnsideKit team). We've asked them to publish it. Once it's on CDN, this subspec approach works as-is with no code changes needed. This PR is intentionally written for that scenario.
There was a problem hiding this comment.
@hyochan, understood — thanks for the context. Since this approach is intentionally forward-looking for when OnsideKit lands on the CDN, that makes sense.
One optional suggestion: it may be worth adding a temporary WarningAggregator.addWarningIOS note inside ensureOnsidePodIOS (or in the docs) to surface a clear message to users who enable Onside before OnsideKit is published — so they get an actionable error rather than a cryptic pod install failure. This can be removed once CDN availability is confirmed.
That said, if you prefer to keep it minimal until the CDN publish is done, that's reasonable too.
✏️ Learnings added
Learnt from: hyochan
Repo: hyochan/expo-iap PR: 321
File: plugin/src/withIAP.ts:267-287
Timestamp: 2026-03-17T16:08:04.425Z
Learning: In `expo-iap`, the `ExpoIap/Onside` subspec in `ios/ExpoIap.podspec` declares `ss.dependency 'OnsideKit'`, but OnsideKit is NOT yet published to the public CocoaPods CDN (as of March 2026). The PR using this subspec approach is intentionally written for when OnsideKit becomes available on CDN — at that point no code changes are needed. The OnsideKit team has been contacted to publish it.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: hyochan/expo-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T23:18:39.882Z
Learning: Applies to ios/ExpoIap.podspec : Never modify the iOS platform version in `ios/ExpoIap.podspec` - it MUST remain at `13.4` due to Expo SDK compatibility, even though code requires iOS 15.0+
Learnt from: CR
Repo: hyochan/expo-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T23:18:39.882Z
Learning: Applies to ios/ExpoIap.podspec : Ensure tvOS platform version in `ios/ExpoIap.podspec` is set to `16.0` (not `13.4`) because the `openiap` CocoaPods dependency requires tvOS 16.0 minimum
Learnt from: CR
Repo: hyochan/expo-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T23:18:39.882Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Functions that only operate on one platform must carry the suffix: `nameIOS` or `nameAndroid` (e.g., `getStorefrontIOS`, `deepLinkToSubscriptionsAndroid`)
Learnt from: CR
Repo: hyochan/expo-iap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T23:18:39.882Z
Learning: Applies to **/*.{ts,tsx} : Inside the `useIAP` hook, most methods return `Promise<void>` and update internal state. Do not design examples or implementations that expect data from methods like `fetchProducts`, `requestPurchase`, `getAvailablePurchases`, `getActiveSubscriptions`. Consume state from hook instead
Learnt from: hyochan
Repo: hyochan/expo-iap PR: 198
File: android/src/main/java/expo/modules/iap/ExpoIapModule.kt:39-49
Timestamp: 2025-09-12T03:46:34.343Z
Learning: In expo-iap codebase, maintainer hyochan prefers simpler implementations over complex safety measures when the additional complexity isn't justified by the specific use case requirements.
2 participants
Summary by CodeRabbit
New Features
Chores
Tests