Skip to content

Improves spm deintegrate script to handle more edge-cases - #57930

Open
ospfranco wants to merge 3 commits into
react:mainfrom
ospfranco:ospfranco/spm-script-improvements
Open

Improves spm deintegrate script to handle more edge-cases#57930
ospfranco wants to merge 3 commits into
react:mainfrom
ospfranco:ospfranco/spm-script-improvements

Conversation

@ospfranco

Copy link
Copy Markdown
Contributor

Summary:

The SPM deintegrate script can fail in subtle ways on certain edge cases. This PR aims to improve:

  • The script only handles single-line use_react_native!(...) calls. If the podfile has been modified then the stripping doesn't work correctly
  • The script does not turn off automaticPodsInstallation in the host app react-native.config.js, without this pods gets re-installed and the SPM project fails
  • The existing .xcworkspace does not get completely cleared of the Pods project, leaving a dangling reference

Changelog:

[IOS] [FIXED] - Fixes various edge-case failures with the spm deintegrate script

Test Plan:

Tests have been added for each of the described edge-cases. For disclosure: this is mostly an AI PR, I just guided claude through the necessary changes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 12, 2026
@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 12, 2026
Comment thread packages/react-native/scripts/setup-apple-spm.js

@cipolleschi cipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for picking this up — all three problems you describe are real, and there are things I like here: the workspace ref is only touched when Pods/Pods.xcodeproj is actually gone from disk, the config editor bails out with a warning instead of guessing on an unrecognized shape, and the new helpers are exported and unit-tested.

I checked the branch out and exercised the exported helpers directly against the community-template Podfile, rn-tester's Podfile and a handful of react-native.config.js shapes. Everything below marked (verified) is something I actually ran, not something I read off the diff.

Requesting changes: as it stands the automaticPodsInstallation fix does not take effect for a standard app layout, and the Podfile stripping regresses a case the old line-filter handled.

Blocking

1. react-native.config.js is written to the wrong directory, so the main fix is a no-op for standard apps. main() redirects appRoot to <projectRoot>/ios for any standard RN layout (L1016-L1023 on main), and runDeintegrate(appRoot) passes that straight to disableAutomaticPodsInstallation. The CLI only ever searches the project root (readConfigFromDisk runs cosmiconfig with stopDir: rootFolder), so the file lands somewhere nothing reads, and the user gets a stray ios/react-native.config.js. Only rn-tester's flat layout (where appRoot === projectRoot) works today. projectRoot is in scope at the setupXcodeproj call site — thread it through and use it for this one operation, while pod deintegrate keeps using appRoot. Inline at the configPath line.

2. Creating react-native.config.js can shadow an existing react-native.config.ts / .cjs. Once #1 is fixed this becomes live: cosmiconfig's search order is ['react-native.config.js', '.cjs', '.ts'], so a newly created .js wins over the user's real config and silently drops their dependencies / commands / platforms. Inline.

3. Neither new mutation is reversible by spm deinit. removeSpmInjection is built around "record every mutation in the marker, undo exactly that". The config edit and the contents.xcworkspacedata edit are recorded nowhere. Concretely: spm add --deintegrate → change of mind → spm deinit → back on CocoaPods, and now react-native run-ios silently never installs pods again, with no hint why. Inline.

4. config = use_native_modules! regresses versus the old line-filter (verified). The old filter dropped the whole line; the new scanner leaves config = dangling, which Ruby folds into the next statement (config = post_install do … end), so config[:reactNativePath] inside that block then blows up. The test asserts this output as correct, so it locks the regression in. Related: the template's post_install / react_native_post_install(installer, config[...]) is left behind either way, so a subsequent pod install — which the docs explicitly tell users to run when non-RN pods remain — still fails. Inline on both the source and the test.

Should fix

5. Silent no-ops in the config editor (verified). Three inputs where the function reports success and the flag is never set — a comment containing } inside project, a quoted 'project': key, and a commented-out automaticPodsInstallation: false. The first two insert a duplicate key that the later one overrides. Details inline; both are cheap to fix without changing the overall approach.

6. Prettier fails on both files (verified) — CI lint will be red. Two spots, inline.

7. Docs not updated. packages/react-native/scripts/spm/__doc__/spm-scripts.md enumerates exactly what --deintegrate does (L44-L67, L117, L136). Two new side effects need to appear there, and the "then run pod install" guidance at L59-L67 now needs a note that automatic pod installs are deliberately off.

8. Placement. ~330 lines of file-format manipulation land in the orchestrator, taking it from 1283 to ~1600 lines. The convention in this directory is that format handling lives in scripts/spm/*.js (read-podspec.js, spm-pbxproj.js, and cleanupLeftoverPodsGroup itself in generate-spm-xcodeproj.js). spm/podfile.js, spm/rn-config.js, spm/xcworkspace.js would keep setup-apple-spm.js orchestrating.

9. Test gaps / test plan. Nothing covers disableAutomaticPodsInstallation itself (creates-when-missing, warn path, and crucially which directory), findXcworkspace's fallback scan, an RN call appearing in a comment, or the .ts/.cjs shadowing case. Given the disclosure that this is largely AI-authored, unit tests alone aren't enough for a script whose whole job is mutating real projects — I'd want the round trip on a fresh app in the test plan: spm add --deintegratereact-native.config.js at the project root → run-ios doesn't re-run pods → no red Pods.xcodeproj row in Xcode → spm deinit → CocoaPods works again.

(For transparency: I couldn't run jest in my checkout — flow-parser is missing there, pre-existing and unrelated to this PR — so I exercised the exported helpers directly with node instead.)

// package graph, the same class of problem `podfileHasRnIntegration` warns
// about for the Podfile itself.
function disableAutomaticPodsInstallation(appRoot /*: string */) /*: void */ {
const configPath = path.join(appRoot, 'react-native.config.js');

@cipolleschi cipolleschi Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — this writes to the wrong directory in the standard app layout.

main() redirects appRoot to <projectRoot>/ios whenever the app has the standard layout (L1016-L1023 on main), and runDeintegrate(appRoot) hands that down here. So this creates/edits <projectRoot>/ios/react-native.config.js.

Only RNTester has this layout, regular apps have the config.js file in the root project folder.

projectRoot is already in scope at the setupXcodeproj call site, so threading it through is the fix. Note pod deintegrate itself must keep running in appRoot — only the config file is project-root-relative.

Worth a test that asserts the file lands next to package.json, not next to the .xcodeproj.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for testing this, how can I test it myself? Are there some instructions on how to run local changes against a project?

'utf8',
);
log(
'Created react-native.config.js with `automaticPodsInstallation: false`.',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once the directory is fixed, creating this unconditionally can silently shadow the user's real config.

cosmiconfig's search order is ['react-native.config.js', 'react-native.config.cjs', 'react-native.config.ts'] (plus .mjs on the async path). A project using react-native.config.ts would keep the file on disk but the newly created .js wins, so their dependencies / commands / platforms config silently stops being applied.

Check for all four names before creating; if the existing one isn't the .js you can edit, fall back to the warn path below rather than adding a second config file.

// lines + closing paren behind, producing a syntactically broken Podfile.
// Only strips the call's own line(s); doesn't touch surrounding code, so a
// call assigned to a variable (`config = use_native_modules!(...)`) keeps its
// line but loses the call — matching prior (single-line) behavior.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This last sentence isn't quite right, and it's where a regression sneaks in: the prior line-filter removed the whole line, config = included. The new scanner removes only the call, so the stock template comes out as (verified):

target 'HelloWorld' do
  config = 

  post_install do |installer|
    react_native_post_install(installer, config[:reactNativePath])
  end
end

ruby -c reports Syntax OK — because Ruby folds it into config = post_install do … end, so config becomes the return value of post_install and config[:reactNativePath] raises inside the block. Consuming an enclosing lhs = when the call is the entire RHS (/^[ \t]*(?:\w+\s*=\s*)?use_native_modules!/) fixes it.

Two related points while you're here:

  • The template also leaves post_install do |installer| react_native_post_install(...) end behind, which references both a removed helper and config. So even after the multi-line fix, a later pod install — the flow the docs tell users to run when non-RN pods remain — still fails. Either strip that block too or scope the PR's claim.
  • Matching with indexOf means comments get mangled: # use_react_native! does X# does X (verified). Anchoring to statement position handles this as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, this one is my fault, didn't check myself the output and was late at night, sorry. Thanks for catching this

}

// Finds the matching `}` for the `{` at `openIdx`, or null if unbalanced.
function matchingBrace(text /*: string */, openIdx /*: number */) /*: number | null */ {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Prettier fails on this signature — it wants the params wrapped:

function matchingBrace(
  text /*: string */,
  openIdx /*: number */,
) /*: number | null */ {

There's a second violation at the "in react-native.config.js (unrecognized format). Set \project.ios." +line below (needs single quotes).yarn prettier --write` on both files clears it — CI lint is red as-is (verified).

// relative to `start` — i.e. a direct property of the object being scanned,
// not a same-named key nested inside some other property's value. Returns
// the `{...}` range of that key's object value, or null if absent.
function findTopLevelKeyObjectRange(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Depth is counted over raw text, so a brace inside a comment or string throws off the whole scan. Verified failure with a perfectly ordinary config:

module.exports = {
  project: {
    // closes the } block
    ios: {sourceDir: './ios'},
  },
};

The stray } in the comment puts the scan at depth -1, ios: { is skipped as "not top-level", and a second ios key is inserted ahead of the real one. The file still parses, the later key wins, and the effective value stays undefined — a silent no-op. A quoted 'project': key does the same thing one level up, because \bproject\s*:\s*{ doesn't match it (verified).

Two in-solution fixes, neither of which changes the approach:

  1. Make the scanner skip string literals and // / /* */ comments — scanToClose in spm/spm-pbxproj.js already does exactly this for pbxproj, so there's a local pattern to copy.
  2. Guard the result: after building the new text, verify the change actually took (the value is reachable at project.ios), and fall back to the warn path if not, instead of writing a file with a duplicate key.

function withAutomaticPodsInstallationDisabled(
contents /*: string */,
) /*: string | null */ {
if (/automaticPodsInstallation\s*:\s*false\b/.test(contents)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These two early paths aren't scoped to project.ios, so they fire on any occurrence anywhere in the file (verified):

  • // automaticPodsInstallation: false, left in a comment → treated as already disabled, real setting never written.
  • Same for an occurrence nested under dependencies.
  • The truefalse replace below rewrites the first match wherever it is, which may not be the one under project.ios.

Since the script already shells out to @react-native-community/cli config and keeps the parsed JSON (CliConfigJson), the authoritative check is right there: project.ios.automaticPodsInstallation is part of that output (cli-config-apple's getProjectConfig), and CliConfigJson.project.ios is already modelled in spm/spm-types.js — one field to add. That gives you the effective value, honours .ts/.cjs/.mjs configs, and is rooted at projectRoot, which also lines up with the directory issue above. The text edit is then only needed for the write, and only when the read says it's still enabled.

appRoot /*: string */,
xcodeprojPath /*: string */,
) /*: string | null */ {
const sibling = path.join(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this readdirSync + $FlowFixMe Dirent block is a copy of the one in resolveInjectionTarget — worth one shared listSubdirsWithSuffix(dir, suffix) helper.

Also, the sibling check uses dirname(xcodeprojPath) while the fallback scans appRoot; those can differ when --xcodeproj points into a subdirectory. Scanning the same directory in both branches would be more predictable.

// the workspace, so this reference dangles — Xcode shows a permanent red,
// missing Pods.xcodeproj row in the workspace navigator otherwise.
function removeDanglingPodsFileRef(xml /*: string */) /*: string */ {
return xml.replace(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: this is the one file here that's machine-generated with a stable shape, so a pattern match is defensible — but it's brittle in ways that are easy to avoid. It won't match a container: prefix, a nested path (group:ios/Pods/Pods.xcodeproj), or reordered attributes.

Matching FileRef elements and filtering on location ending in Pods/Pods.xcodeproj would be about the same amount of code and wouldn't care about the surrounding formatting.

if (cleanupLeftoverPodsGroup(xcodeprojPath)) {
log('Removed the leftover empty `Pods` group from the project.');
}
if (cleanupDanglingPodsWorkspaceRef(appRoot, xcodeprojPath)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — this mutation, and the react-native.config.js one, aren't undone by spm deinit.

removeSpmInjection is deliberately built as "record every mutation in the marker, undo exactly that". Neither of the two new side effects is recorded there, so:

spm add --deintegrate → user changes their mind → spm deinit → they're back on CocoaPods, except automaticPodsInstallation is still false, so react-native run-ios silently never installs pods again and there's nothing pointing at why.

Please record both in the marker and restore them on deinit. If full restore is out of scope for this PR, at least surface it in the deinit output so the user knows to flip the flag back.

expect(stripped).not.toMatch(/^\s*\)\s*$/m);
expect(stripped).toBe(
"target 'HelloWorld' do\n" +
' config = \n' +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This asserts the dangling config = as expected output, which locks in the regression noted on the source side — the old line-filter removed that line entirely, and Ruby folds config = into the following post_install do … end.

Once the enclosing assignment is consumed, this expectation should be "target 'HelloWorld' do\n\n target 'HelloWorldTests' do\n…".

Two cases worth adding while you're in here: the RN call appearing inside a comment (# use_react_native! does X, currently mangled to # does X), and the full stock template including its post_install block, asserting the result is something pod install can still consume.

@ospfranco

Copy link
Copy Markdown
Contributor Author

I did one change, instead of completely leaving the post_install hook as-is, at least tries to strip the default hook. If anything has been modified, it is left as-is and the user should remove/update it manually

@ospfranco
ospfranco requested a review from cipolleschi August 13, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants