-
Notifications
You must be signed in to change notification settings - Fork 7
Convert binary plist files to xml and use a proper parser when linking #275
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces manual plist string manipulation with the @expo/plist parser library and adds support for converting binary plist files to XML format before modification. This addresses issue #126 by providing proper plist parsing and handling of binary plist formats.
Key changes:
- Replaced custom
createPlistContent()with@expo/plistlibrary'sbuild()method - Modified
readInfoPlist()to accept a file path parameter and useplist.parse()instead of returning raw string contents - Added binary-to-XML conversion step in
updateInfoPlist()using theplutilcommand-line tool
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/host/package.json | Added @expo/plist dependency |
| packages/host/src/node/prebuilds/apple.ts | Replaced custom plist generation with @expo/plist library |
| packages/host/src/node/cli/apple.ts | Refactored plist reading/updating to use proper parser and handle binary format conversion |
| packages/host/src/node/cli/apple.test.ts | Updated tests to reflect API changes and added binary plist conversion test |
| // Convert to XML format if needed | ||
| await spawn("plutil", ["-convert", "xml1", infoPlistPath], { | ||
| outputMode: "inherit", | ||
| }); | ||
| const contents = await readInfoPlist(infoPlistPath); |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plutil conversion command will silently succeed even if the file is already in XML format, but if it fails (e.g., corrupted file, invalid plist), the error won't be captured effectively with outputMode: "inherit". Consider adding error handling or validation after the conversion to ensure the plist is in a valid state before attempting to parse it.
| // Convert to XML format if needed | |
| await spawn("plutil", ["-convert", "xml1", infoPlistPath], { | |
| outputMode: "inherit", | |
| }); | |
| const contents = await readInfoPlist(infoPlistPath); | |
| // Convert to XML format if needed, and validate conversion | |
| try { | |
| await spawn("plutil", ["-convert", "xml1", infoPlistPath], { | |
| outputMode: "inherit", | |
| }); | |
| } catch (plutilError) { | |
| throw new Error(`Failed to convert Info.plist to XML format at "${infoPlistPath}".`, { cause: plutilError }); | |
| } | |
| let contents: Record<string, unknown>; | |
| try { | |
| contents = await readInfoPlist(infoPlistPath); | |
| } catch (parseError) { | |
| throw new Error(`Info.plist at "${infoPlistPath}" is not a valid plist after conversion.`, { cause: parseError }); | |
| } |
packages/host/src/node/cli/apple.ts
Outdated
| }); | ||
| const contents = await readInfoPlist(infoPlistPath); | ||
| if (contents.CFBundleExecutable === oldLibraryName) { | ||
| contents.CFBundleExecutable = newLibraryName; |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional check prevents updating if CFBundleExecutable doesn't match oldLibraryName, but this silently skips the update without any indication. Consider logging a warning or throwing an error if the expected value doesn't match, as this could indicate an unexpected plist state or incorrect parameters.
| contents.CFBundleExecutable = newLibraryName; | |
| contents.CFBundleExecutable = newLibraryName; | |
| } else { | |
| throw new Error( | |
| `CFBundleExecutable in Info.plist at "${infoPlistPath}" is "${contents.CFBundleExecutable}", expected "${oldLibraryName}". Aborting update.` | |
| ); |
| const binaryPlistContents = Buffer.from( | ||
| "YnBsaXN0MDDfEBUBAgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4cICEiIyQiJSYnJChfEBNCdWlsZE1hY2hpbmVPU0J1aWxkXxAZQ0ZCdW5kbGVEZXZlbG9wbWVudFJlZ2lvbl8QEkNGQnVuZGxlRXhlY3V0YWJsZV8QEkNGQnVuZGxlSWRlbnRpZmllcl8QHUNGQnVuZGxlSW5mb0RpY3Rpb25hcnlWZXJzaW9uXxATQ0ZCdW5kbGVQYWNrYWdlVHlwZV8QGkNGQnVuZGxlU2hvcnRWZXJzaW9uU3RyaW5nXxARQ0ZCdW5kbGVTaWduYXR1cmVfEBpDRkJ1bmRsZVN1cHBvcnRlZFBsYXRmb3Jtc18QD0NGQnVuZGxlVmVyc2lvbl8QFUNTUmVzb3VyY2VzRmlsZU1hcHBlZFpEVENvbXBpbGVyXxAPRFRQbGF0Zm9ybUJ1aWxkXkRUUGxhdGZvcm1OYW1lXxARRFRQbGF0Zm9ybVZlcnNpb25aRFRTREtCdWlsZFlEVFNES05hbWVXRFRYY29kZVxEVFhjb2RlQnVpbGRfEBBNaW5pbXVtT1NWZXJzaW9uXlVJRGV2aWNlRmFtaWx5VjI0RzIzMVdFbmdsaXNoVWFkZG9uXxAPZXhhbXBsZV82LmFkZG9uUzYuMFRGTVdLUzEuMFQ/Pz8/oR9fEA9pUGhvbmVTaW11bGF0b3IJXxAiY29tLmFwcGxlLmNvbXBpbGVycy5sbHZtLmNsYW5nLjFfMFYyMkMxNDZfEA9pcGhvbmVzaW11bGF0b3JUMTguMl8QE2lwaG9uZXNpbXVsYXRvcjE4LjJUMTYyMFgxNkM1MDMyYaEpEAEACAA1AEsAZwB8AJEAsQDHAOQA+AEVAScBPwFKAVwBawF/AYoBlAGcAakBvAHLAdIB2gHgAfIB9gH7Af8CBAIGAhgCGQI+AkUCVwJcAnICdwKAAoIAAAAAAAACAQAAAAAAAAAqAAAAAAAAAAAAAAAAAAAChA==", | ||
| "base64", | ||
| ); |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base64-encoded binary plist lacks documentation explaining what it represents or how it was generated. Add a comment describing the plist's content (e.g., 'Binary plist containing CFBundleExecutable: "addon"') and how to regenerate it if needed, to improve test maintainability.
b85ffa9 to
c6fbffd
Compare
c6fbffd to
eb8ac71
Compare
Fixes #126 by using a proper plist parser (
@expo/plist) and also handles converting binary plists to xml before updating.