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

fix: fix and enhance AGP namespace support #1044

Merged
merged 5 commits into from
Oct 30, 2023
Merged

Conversation

a7medev
Copy link
Contributor

@a7medev a7medev commented Oct 25, 2023

Description of the change

The current implementation of Android Gradle Plugin >= 7.3.0 namespace support calculates isManifestContentUpdated before applying any modifications to the file, so the manifest file was never updated since isManifestContentUpdated will always be false. The fix was to calculate it right before writing the updates to the file.

I've changed the namespace support condition to check for AGP version >= 7.3 not only >= 7 since namespace support was added in v7.3.0. I've also refactored the code into functions and renamed some variables for readability.

I've also kept the package property in the initial AndroidManifest.xml file since having it removed by default causes the first app build to fail sometimes with apps not using AGP >= 7.3.0, while having it there in the initial run when it's not needed (when using namespace) won't cause trouble.

I've tested these changes on React Native 0.66.0 and 0.73.0-rc.3 and its working fine.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Jira ID: INSD-10287

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests

Code review

  • This pull request has a descriptive title and information useful to a reviewer
  • Issue from task tracker has a link to this pull request

The current implementation calculates `isManifestContentUpdated` before applying any modifications to the file, so the manifest file was never updated. The fix was to calculate it right before updating the file.
@a7medev a7medev self-assigned this Oct 25, 2023
@InstabugCI
Copy link
Collaborator

Coverage Report

Label Coverage Status
JavaScript 95.8%
Android 50.3%
iOS 40.5%

Generated by 🚫 dangerJS against d381803

Copy link
Contributor

@abdelhamid-f-nasser abdelhamid-f-nasser left a comment

Choose a reason for hiding this comment

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

The code looks excellent and is very self-explanatory 💪 !! Great effort supporting it @a7medev 👍

@a7medev a7medev merged commit b9aeb94 into dev Oct 30, 2023
@a7medev a7medev deleted the fix/agp-namespace-support branch October 30, 2023 11:24
abdelhamid-f-nasser pushed a commit that referenced this pull request Nov 1, 2023
* fix: check for manifest updates before updating

The current implementation calculates `isManifestContentUpdated` before applying any modifications to the file, so the manifest file was never updated. The fix was to calculate it right before updating the file.

* refactor: enhance AGP namespace support

* fix: resolve groovy compile issues

* chore(android): keep package in initial manifest

* chore: update changelog
HeshamMegid pushed a commit that referenced this pull request Nov 14, 2023
* fix: check for manifest updates before updating

The current implementation calculates `isManifestContentUpdated` before applying any modifications to the file, so the manifest file was never updated. The fix was to calculate it right before updating the file.

* refactor: enhance AGP namespace support

* fix: resolve groovy compile issues

* chore(android): keep package in initial manifest

* chore: update changelog
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.

3 participants