Skip to content

fix(ios): let the app's xcconfig win over a plugin's, and warn on discarded settings - #6091

Merged
NathanWalker merged 4 commits into
NativeScript:mainfrom
edusperoni:fix/xcconfig-app-precedence
Jul 28, 2026
Merged

fix(ios): let the app's xcconfig win over a plugin's, and warn on discarded settings#6091
NathanWalker merged 4 commits into
NativeScript:mainfrom
edusperoni:fix/xcconfig-app-precedence

Conversation

@edusperoni

@edusperoni edusperoni commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

An app currently has no way to override a build setting that a plugin declares, and no way to find out that it failed to.

The app's xcconfig never wins

XcconfigService.mergeFiles keeps whichever value is already in the destination and deletes the incoming one:

userConfig.attributes.each do |key,|
  existingConfig.attributes.delete(key) if (userConfig.attributes.key?(key) && existingConfig.attributes.key?(key))
end
userConfig.merge(existingConfig)

First writer wins — and mergeProjectXcconfigFiles merged every plugin's platforms/ios/build.xcconfig before the app's App_Resources/iOS/build.xcconfig. So for any key a plugin set, the app's value was discarded outright. Not "sometimes lost": never applied. Among plugins the winner was whichever came first in dependency-resolution order, which nobody controls.

This PR merges the app's file first. Plugins still supply every key the app says nothing about; they just stop overriding the ones it does.

Where this bites today: V8 14.9 (NativeScript iOS 9) made v8config.h reject C++17 outright —

#if __cplusplus <= 201703L
#error "C++20 or later required."
#endif

— while several plugins pin CLANG_CXX_LANGUAGE_STANDARD = c++17, which sat exactly on the old floor. One stale transitive plugin holds the whole app below the floor, and the app cannot lift it. The resulting error points at the runtime's headers rather than at the plugin that pinned the standard.

Discarded settings are now reported

The merge dropped keys in silence, so a discarded setting was indistinguishable from one that was never written. That's the reason the case above is hard to diagnose: nothing tells you which plugin pinned the value, and the failure surfaces somewhere else entirely.

mergeFiles now reports what it dropped:

Ignoring CLANG_CXX_LANGUAGE_STANDARD = c++17 from node_modules/some-plugin/platforms/ios/build.xcconfig:
already set to c++20 by a higher precedence xcconfig. The app's App_Resources xcconfig wins over any plugin's.

Only conflicts whose values genuinely differ are reported — two files agreeing on a key is not worth a warning, and that keeps this quiet on normal projects. Reporting is also fully isolated from the merge: an unparseable result is traced and ignored, never thrown.

Compatibility

The precedence change is behavioural. An app that sets a key a plugin also sets now gets its own value where it previously got the plugin's. That is the intent, but it can surface a latent misconfiguration — a value set long ago that silently never applied and now does. The new warning makes those visible rather than mysterious.

Conflicts between plugins are unchanged: still resolved by dependency order, still arbitrary. xcconfig can't express "highest wins" for an ordered value like a language standard, so warning is the practical ceiling; this PR at least makes them audible.

Tests

npm test — 1514 passing, 9 pending, 0 failures.

Added The app's build.xcconfig wins over a plugin's, which sets CLANG_CXX_LANGUAGE_STANDARD in both an app and a plugin xcconfig and asserts the app's value survives while a plugin-only key still lands. It runs the real xcodeproj merge rather than a stub, so it also covers the modified Ruby.

test/xcconfig-service.ts gains a logger registration for the new dependency.

Summary by CodeRabbit

  • Bug Fixes

    • Updated iOS xcconfig merging so app App_Resources settings are authoritative over plugin-provided values when conflicts occur.
    • Added warnings for detected xcconfig key conflicts, while continuing to preserve the higher-priority (app) values.
  • Tests

    • Added a merge test covering precedence for conflicting CLANG_CXX_LANGUAGE_STANDARD, ensuring app-pinned values win while unrelated plugin settings (e.g., GCC_C_LANGUAGE_STANDARD) are retained.

XcconfigService.mergeFiles keeps whichever value is already in the
destination and drops the incoming one, and mergeProjectXcconfigFiles
merged every plugin before the app's App_Resources/iOS/build.xcconfig. So
for any key a plugin set, the app's value was discarded outright -- there
was no way to override a plugin from the app, and among plugins the winner
was whichever came first in dependency resolution order.

Merging the app's file first makes it authoritative while leaving plugins
to supply everything the app says nothing about.
A dropped key was indistinguishable from one that was never written, so a
plugin pinning something like CLANG_CXX_LANGUAGE_STANDARD could hold an
entire app below a required standard with no indication of which plugin
was responsible.

Only conflicts whose values actually differ are reported; two files
agreeing on a key is not worth a warning.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 124c9a2f-267f-4128-a3d2-15f3fc9e65e5

📥 Commits

Reviewing files that changed from the base of the PR and between f219cd5 and 8b839a2.

📒 Files selected for processing (1)
  • lib/services/xcconfig-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/services/xcconfig-service.ts

📝 Walkthrough

Walkthrough

XcconfigService now detects and logs conflicting settings while retaining destination values. Project merging applies app XCConfig values before plugin values, and tests cover precedence and logger injection.

Changes

XCConfig merge behavior

Layer / File(s) Summary
Conflict-aware XCConfig merging
lib/services/xcconfig-service.ts, test/xcconfig-service.ts
XcconfigService accepts a logger, reports differing duplicate settings, and preserves destination values during merges.
App-over-plugin merge ordering
lib/services/ios-project-service.ts, test/ios-project-service.ts
App build.xcconfig values are merged before plugin values, preserving app overrides and plugin-only settings.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant XcconfigService
  participant RubyMerge
  participant Logger
  XcconfigService->>RubyMerge: merge destination and source xcconfig files
  RubyMerge-->>XcconfigService: return merged output with conflict JSON
  XcconfigService->>Logger: log warnings for differing settings
Loading

Possibly related PRs

Poem

I’m a bunny guarding settings tight,
App values hop ahead in line.
Plugin keys join when they’re new,
Conflicts leave warnings bright and true.
XCConfig merges just right!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: app xcconfig precedence over plugin settings plus conflict warnings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
test/xcconfig-service.ts (1)

22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add direct coverage for conflict reporting.

Use LoggerStub to assert: differing values emit one warning, equivalent values emit none, and malformed marker JSON only emits a trace without rejecting the merge.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/xcconfig-service.ts` at line 22, Add direct tests around the
conflict-reporting merge flow using the registered LoggerStub: verify differing
values produce exactly one warning, equivalent values produce no warnings, and
malformed marker JSON produces only a trace while allowing the merge to complete
successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/services/xcconfig-service.ts`:
- Around line 93-98: Remove the unconditional App_Resources attribution from the
warning assembled in the conflicts loop of the xcconfig merge logic. Keep the
existing retained-value and higher-precedence message so plugin-vs-plugin
conflicts accurately reflect dependency-order precedence without claiming the
app won.
- Around line 46-68: Update the XCConfig merge execution around mergeScript and
$childProcess.exec to use execFile with separate Ruby arguments instead of
shell-interpolating a ruby -e command. Pass the merge script and
destination/source paths as arguments, read the paths from Ruby’s ARGV, and
remove the apostrophe-based path interpolation so command substitution cannot
execute.

---

Nitpick comments:
In `@test/xcconfig-service.ts`:
- Line 22: Add direct tests around the conflict-reporting merge flow using the
registered LoggerStub: verify differing values produce exactly one warning,
equivalent values produce no warnings, and malformed marker JSON produces only a
trace while allowing the merge to complete successfully.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c89f567-321c-4776-9de2-4183c3b6187f

📥 Commits

Reviewing files that changed from the base of the PR and between 354903e and f219cd5.

📒 Files selected for processing (4)
  • lib/services/ios-project-service.ts
  • lib/services/xcconfig-service.ts
  • test/ios-project-service.ts
  • test/xcconfig-service.ts

Comment thread lib/services/xcconfig-service.ts Outdated
Comment thread lib/services/xcconfig-service.ts
NathanWalker and others added 2 commits July 28, 2026 12:56
The merge ran through a shell-interpolated `ruby -e "..."` with only
apostrophes escaped, so a project or node_modules path containing a
command substitution was executed by the shell before ruby started. A
directory named `a$(hostname)b` is enough to reproduce it.

execFile passes the script and both paths as arguments, which removes the
shell from the path entirely and makes the escaping unnecessary.

Also corrects the conflict warning: it claimed the app's xcconfig had won,
which is wrong for a plugin-vs-plugin conflict, where the earlier plugin
wins by dependency order. It now states the ordering instead of naming a
winner.
@NathanWalker
NathanWalker merged commit 1b5cabd into NativeScript:main Jul 28, 2026
7 checks passed
@edusperoni edusperoni mentioned this pull request Jul 28, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants