Improves spm deintegrate script to handle more edge-cases - #57930
Improves spm deintegrate script to handle more edge-cases#57930ospfranco wants to merge 10 commits into
Conversation
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
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 --deintegrate → react-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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`.', |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
endruby -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(...) endbehind, which references both a removed helper andconfig. So even after the multi-line fix, a laterpod 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
indexOfmeans comments get mangled:# use_react_native! does X→# does X(verified). Anchoring to statement position handles this as well.
There was a problem hiding this comment.
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 */ { |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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:
- Make the scanner skip string literals and
////* */comments —scanToCloseinspm/spm-pbxproj.jsalready does exactly this for pbxproj, so there's a local pattern to copy. - 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)) { |
There was a problem hiding this comment.
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
true→falsereplace below rewrites the first match wherever it is, which may not be the one underproject.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( |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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' + |
There was a problem hiding this comment.
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.
|
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 |
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
Thanks for the follow-up — several earlier concerns are addressed, including the project-root config location, alternate config-file detection, Podfile cleanup, formatting, and documentation. I found a few remaining config and rollback edge cases inline. The focused Jest suite passes (90/90), but I could reproduce the silent config no-op and non-exact round trip described below.
Please also rebase onto main; GitHub currently reports merge conflicts. Happy to take another look after these are addressed.
| projectRange.open + 1, | ||
| projectRange.close, | ||
| ); | ||
| if (iosRange == null) { |
There was a problem hiding this comment.
Could we fail closed when an ios key exists but is not an object literal? For a valid config such as const iosConfig = {...}; module.exports = {project: {ios: iosConfig}}, this inserts a second ios property before the original. The later property wins, so the helper reports success while automatic pod installation remains enabled. Braces inside string values can produce the same silent no-op. Checking for any existing project/ios key before inserting, then verifying through the effective CLI config, would make this safer.
There was a problem hiding this comment.
Let me know if you really want to verify through the cli config. That seems like a bigger change, instead of this best effort simple text parsing.
| // recorded. Read back by `spm deinit` (removeSpmInjection, below) to | ||
| // restore project.ios.automaticPodsInstallation. | ||
| const automaticPodsInstallation = | ||
| opts.automaticPodsInstallation ?? |
There was a problem hiding this comment.
Could this preserve the original mutation record across repeated --deintegrate runs? The first run records created/edited, but a second run returns already-disabled and overwrites that record here. spm deinit then does nothing and leaves the flag disabled. Keeping the previous record when the current result made no change should preserve rollback.
| return; | ||
| } | ||
| const orig = fs.readFileSync(result.configPath, 'utf8'); | ||
| const restored = withAutomaticPodsInstallationEnabled(orig); |
There was a problem hiding this comment.
This cannot distinguish an original true from an originally absent property. In the absent case, deinit leaves a newly inserted automaticPodsInstallation: true (and any inserted project/ios structure), so the file is not restored to its prior state. Could the marker record the specific edit—or the original content plus a safety hash—and restore only when unchanged?
| if (cleaned === orig) { | ||
| return false; | ||
| } | ||
| fs.writeFileSync(dataPath, cleaned, 'utf8'); |
There was a problem hiding this comment.
This workspace edit is still not recorded for deinit. Since the docs describe the migration as fully reversible and deinit as the exact inverse, could we record and safely restore this FileRef, or explicitly document that this part requires manual restoration?
There was a problem hiding this comment.
I thought a pod install would restore the reference, but sure, better if the script cleanly applies all the changes in one go
| // whatever a prior `add --deintegrate` recorded. | ||
| let automaticPodsInstallation /*: ?AutomaticPodsInstallationResult */ = null; | ||
|
|
||
| if (args.deintegrate) { |
There was a problem hiding this comment.
Could the confirmation and injection preflight happen before these mutations? At present deintegration, config editing, and workspace cleanup occur before the dirty-project prompt and before the marker is written. If the user declines or injection fails, partial changes remain with no rollback record.
| return contents; | ||
| } else if (existing.value === 'true') { | ||
| updated = | ||
| contents.slice(0, existing.matchStart) + |
There was a problem hiding this comment.
Small data-preservation edge case: when this is the final property without a comma and has an inline // comment, matchEnd includes the masked comment and this replacement deletes it. Restricting the replacement range to the boolean token would preserve the user's comment.
Prettier alignment fix for the doc-conflict resolution while rebasing onto main.
7893039 to
7c0af2a
Compare
2 participants
Summary:
The SPM deintegrate script can fail in subtle ways on certain edge cases. This PR aims to improve:
automaticPodsInstallationin the host app react-native.config.js, without this pods gets re-installed and the SPM project fails.xcworkspacedoes not get completely cleared of the Pods project, leaving a dangling referenceChangelog:
[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.