Skip to content

Conversation

@kowczarz
Copy link
Contributor

Order of adding targets described in podfile does matters. In previous versions of the plugin Clip target was added inside the main target. In #48 that was changed and the Clip target was separated and it caused the change in the build scripts phase. In our project we have some custom build scripts which are based on previous behaviour, and the 0.4.0 update caused many issues by that minor change.

In my PR I'm getting back to using merge contents, but my anchor is much safer, because it's very unlikely expo will get rid of use_expo_modules! and just in case they will add something to the call, we are using offset 0, what causes the contents are merged just before the mentioned line.

@kowczarz kowczarz changed the title Use mergeContents instead of appending in withPodfile Use mergeContents instead of appending code to the end of file in withPodfile Dec 10, 2024
Copy link
Collaborator

@nathan-ahn nathan-ahn left a comment

Choose a reason for hiding this comment

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

Good catch and thank you for the fix! I didn't realize that the targets were nested due to how Podfile was indented when I first implemented the merge.

On that note, can you revise this PR to indent the contents of appClipTarget? I've been trying to maintain consistent indentation within the Podfile to avoid confusion around start and end blocks.

I've tested this change locally and it seems to be non-breaking. Once this is merged in, we can make this fix the v0.5.1 release. Are there any breaking changes caused by this that I'm not seeing here?

@kowczarz kowczarz requested a review from nathan-ahn December 11, 2024 11:03
@nathan-ahn nathan-ahn merged commit 0d5bd2a into bndkt:main Dec 12, 2024
1 check passed
@kowczarz kowczarz deleted the update-with-podfile branch December 12, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants