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

[Android] Added support for BoM imports #1311

Merged
merged 2 commits into from May 18, 2022

Conversation

ebhsgit
Copy link
Contributor

@ebhsgit ebhsgit commented Aug 5, 2021

Platforms affected

Android

Motivation and Context

Bill of Material (BoM) pattern simplifies the management of dependency versions (when there are multiple packages with shared dependencies).
https://docs.gradle.org/current/userguide/platforms.html#sub:bom_import

A cordova plugin may want to use BoM to manage the versions of it's dependencies.

Currently there is no way to implement this:

  • Package import can not be defined without a version defined
  • No ability to import BoM platform

Example using Firebase BoM
https://firebase.google.com/docs/android/learn-more#bom

Description

  • propertiesObj.systemLibs regex - exclude any imports that contains ( in it's package name
  • Added propertiesObj.bomPlatforms - any value package which matches platform("...")
  • Allow package to be imported without version. This will generate a warning in the console

Testing

I was able to compile my project (with custom plugin) which uses Firebase BoM

Checklist

  • [ x ] I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • [ x ] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • 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)
  • I've updated the documentation if necessary

https://docs.gradle.org/current/userguide/platforms.html#sub:bom_import

Changes

* propertiesObj.systemLibs regex - exclude the value contains (
* added propertiesObj.bomPlatforms - any value which matches platform("...")
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

Merging #1311 (250557f) into master (5db8508) will decrease coverage by 0.24%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1311      +/-   ##
==========================================
- Coverage   73.25%   73.00%   -0.25%     
==========================================
  Files          21       21              
  Lines        1645     1652       +7     
==========================================
+ Hits         1205     1206       +1     
- Misses        440      446       +6     
Impacted Files Coverage Δ
lib/builders/ProjectBuilder.js 70.37% <14.28%> (-2.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5db8508...250557f. Read the comment docs.

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Aug 17, 2021

@breautek
Hi mate, Any way to get someone to review the PR?

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

lgtm

But please be patient on an actual merge -- Not sure if there will be another minor release or not for 10.x.

Copy link
Contributor

@PieterVanPoyer PieterVanPoyer left a comment

Choose a reason for hiding this comment

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

Hey

Is it possible to add some info about how to use this in a plugin?
Maybe some sample configuration/code from your plugin could clarify this a little bit?

Kind regards
Pieter

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Sep 9, 2021

@PieterVanPoyer No problems

Below is the key snippet that this PR would enable

    <platform name="android">
          
          // Other stuff

        <preference name="FIREBASE_BOM" default="28.3.0" />

        <framework src="platform('com.google.firebase:firebase-bom:$FIREBASE_BOM')" />
        <framework src="com.google.firebase:firebase-auth" />
        <framework src="com.google.firebase:firebase-firestore"/>
        <framework src="com.google.firebase:firebase-messaging"/>

         // Other stuff

    </platform>

I've attached an example plugin.xml which enables firebase dependencies using BoM

plugin.zip

@breautek breautek added this to the 11.0.0 milestone Oct 16, 2021
@breautek breautek added this to To Do in Release Plan - 11.0.0 via automation Mar 16, 2022
@breautek
Copy link
Contributor

We have 2 green checks and master is in 11.x dev state so merging!

Thank you @ebhsgit for your contribution and effort into preparing this PR.

@breautek breautek merged commit bd0c8ce into apache:master May 18, 2022
Release Plan - 11.0.0 automation moved this from To Do to Done May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants