Uh oh!
There was an error while loading. Please reload this page.
fix(platform-ios): detect Podfile and .xcworkspace - #1436
Conversation
632e75c to
9d7c292Comparetido64
commented
Jul 5, 2021
grabbou
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left a series of comments.
Before going to addressing them, I wanted to better understand the nature of this PR.
In what scenario, an xcworkspace will be present, but xcodeproj not?
Why should we allow CLI to detect projects that don't have .xcworkspace and .xcodeproj, but have a Podfile only? CLI is designed to support projects, not dependencies.
| * Finds iOS project by looking for all .xcodeproj files | ||
| * in given folder. | ||
| * | ||
| * Returns first match if files are found or null |
There was a problem hiding this comment.
I think the description needs to be updated. How does it work now? Returns first element for which isXcodeProject returns true, or first element in an array? When exactly first element in an array is a valid return value?
| * Note: `./ios/Podfile` are returned regardless of the name | ||
| */ | ||
| export function findPodfile(folder: string): string | null { | ||
| const projects = findProject(folder, '**/Podfile'); |
There was a problem hiding this comment.
Rename projects to podfiles or pods?
| export default function findProject(folder: string): string | null { | ||
| const projects = glob | ||
| .sync(GLOB_PATTERN, { | ||
| function findProject(folder: string, pattern: string): string[] { |
There was a problem hiding this comment.
I guess this function can now be renamed to glob or findInFolder, since it no longer finds a project, but acts as a utility function.
| @@ -48,10 +41,37 @@ export default function findProject(folder: string): string | null { | |||
| path.dirname(project) === IOS_BASE || !TEST_PROJECTS.test(project), | |||
There was a problem hiding this comment.
This one is not related to your PR per se, but I stared wondering whether path.dirname(project) === IOS_BASE should be there or not.
Now, I don't remember why I ended up leaving this one here, instead of using glob pattern ios/**/*.{xcodeproj,xcworkspace}.
I actually wonder why do we enforce iOS folder in first place here. That was probably to ignore node_modules and additional projects that might be there.
| projectPath && isXcodeProject(projectPath) | ||
| ? path.join(projectPath, 'project.pbxproj') | ||
| : null, | ||
| podfile: podfile && path.resolve(podfile), |
There was a problem hiding this comment.
Do we need podfile && here, if we enforce it on line 49?
There was a problem hiding this comment.
Hm right, with !project && !podfile guard we don't need to check either of those
| if (!project) { | ||
| if (!project && !podfile) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Shouldn't this be !project || !podfile instead? I guess a project is not valid when .xcodeproj is missing.
On a side note, I guess with this change, having a Podfile will be mandatory.
grabbou
commented
Jul 6, 2021
@tido64 how about we hop on a call at some point this or next week to discuss this? |
tido64
commented
Jul 6, 2021
I filed a feature request on this last year or so here: #1054. Technically, only A more specific example are projects using |
tido64
commented
Jul 6, 2021
Yes, we can do that as well. I know you're super busy so let me know when you have time? |
grabbou
commented
Jul 6, 2021
I will reach out to you directly over Discord and we will send a note when we're done |
Summary:
react-native configfails to recognize projects that contain only a Podfile or.xcworkspace, e.g. because the Xcode project is generated. This change makes it output a config if either are found.Resolves#1435
Resolves the iOS part of #1054
Test Plan: