Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: Consistent app template/Xcode project name #1485

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Aug 30, 2024

Platforms affected

iOS

Motivation and Context

Currently, the generated Xcode project uses the application name for both the project and the target name. This means that file paths in generated projects are not consistent and we have to do regex-based token replacements during project creation. It also makes modifying the template project a pain because opening the template project directly in Xcode causes a bunch of those tokens to break.

The deeper problem is that some tooling (notably Cocoapods) struggles with project and target names with non-ASCII characters.

Possibly closes GH-664.
Mostly closes GH-717 (still impacted by CocoaPods/CocoaPods#7546).
Possibly closes GH-764.
Probably closes GH-793.
Closes GH-795.
Closes GH-826 (due to not needing to do dynamic Info.plist lookups).
Closes GH-955.
Possibly closes GH-1049.
Closes GH-1150.
Closes GH-1289.
Closes GH-1464.

Description

Remaining work:

  • Set the PRODUCT_NAME to the config.xml name value so that the resulting app/ipa file has the expected name
  • See if we can set PRODUCT_BUNDLE_IDENTIFIER in a better way at create-time to avoid token replacement
  • Test with Cocoapods to make sure they still work as expected
  • Test with Cocoapods and non-ASCII project names to ensure everything works
  • Add a validation check for App.xcodeproj to guard against outdated project files

Will handle as a follow-up PR:

Testing

All unit tests pass.
Tested manually with mobilespec.
Tested manually installing onesignal-cordova-plugin in a project with both ASCII and non-ASCII names (ref GH-1289)
Tested manually installing cc.fovea.cordova.openwith in a project and confirmed it built (ref GH-955)

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 80.89%. Comparing base (98a3bcd) to head (c8b28f3).

Files with missing lines Patch % Lines
lib/build.js 76.92% 3 Missing ⚠️
lib/run.js 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
- Coverage   81.56%   80.89%   -0.68%     
==========================================
  Files          16       16              
  Lines        1888     1842      -46     
==========================================
- Hits         1540     1490      -50     
- Misses        348      352       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dpogue dpogue added this to the 8.0.0 milestone Sep 1, 2024
@dpogue dpogue force-pushed the app-template branch 4 times, most recently from a110c49 to 7bcd9b3 Compare September 27, 2024 06:02
@dpogue dpogue changed the title [WIP] refactor!: Consistent app template/Xcode project name refactor!: Consistent app template/Xcode project name Sep 27, 2024
@dpogue dpogue marked this pull request as ready for review September 27, 2024 06:30
dpogue added 7 commits October 7, 2024 13:22
Otherwise when Xcode consolidates the plist file it will probably
overwrite the ones in the plist with the ones defined in the Xcodeproj
file (which is where it wants to put things by default nowadays).
Some plugins look up the root group of the Xcode project based on the
name "CustomTemplate", which isn't really an ideal thing to do but I'm
unsure if there are better options and this name doesn't actually get
displayed anywhere so we might as well keep the label for compatibility
reasons.
This is a guard against running a newer version of Cordova iOS tools
against an older project with a different name (which will fail due to
now-hardcoded assumptions about the project name).
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

LGTM

Built & launch mobile spec test app. Ran test against default plugins.

@dpogue dpogue merged commit a8f9c57 into apache:master Oct 8, 2024
10 checks passed
@dpogue dpogue deleted the app-template branch October 8, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment