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

Allow Mapbox or MapLibre from SPM on iOS, and different libs on android #1311

Merged
merged 2 commits into from
Apr 17, 2021

Conversation

mfazekas
Copy link
Contributor

@mfazekas mfazekas commented Apr 14, 2021

Description

This PR allows to specify the Mapbox library to be consumed via SwiftPackageManager. This means MapLibre for example can be used instead of Mapbox.

Fixes #1273

Podfile:

$RNMBGL_Use_SPM = {
  url: "https://github.com/maplibre/maplibre-gl-native-distribution",
  requirement: {
    kind: "upToNextMajorVersion",
    minimumVersion: "5.11.0"
  },
  product_name: "Mapbox"
}


pre_install do |installer|
  $RNMBGL.pre_install(installer)
end
...

post_install do |installer|
   flipper_post_install(installer)
   $RNMBGL.post_install(installer)
end

Top level build.gradle:

buildscript {
    ext {
   ...
         rnmbglMapboxLibs = {
             implementation ("org.maplibre.gl:android-sdk:9.2.1")
             implementation ("com.mapbox.mapboxsdk:mapbox-sdk-turf:5.3.0")
         }

         rnmbglMapboxPlugins = {
             implementation ("com.mapbox.mapboxsdk:mapbox-android-gestures:0.7.0")
             implementation ("com.mapbox.mapboxsdk:mapbox-android-plugin-localization-v9:0.12.0")    {
                 exclude group: 'com.mapbox.mapboxsdk', module: 'mapbox-android-sdk'
             }
             implementation ("com.mapbox.mapboxsdk:mapbox-android-plugin-annotation-v9:0.8.0")        {
                 exclude group: 'com.mapbox.mapboxsdk', module: 'mapbox-android-sdk'
             }
             implementation ("com.mapbox.mapboxsdk:mapbox-android-plugin-markerview-v9:0.4.0") {
                 exclude group: 'com.mapbox.mapboxsdk', module: 'mapbox-android-sdk'
              }
         }
}

@mfazekas mfazekas changed the title [iOS] Allow Mapbox or MapLibre from SPM Allow Mapbox or MapLibre from SPM on iOS, and different libs on android Apr 16, 2021
Copy link
Member

@ferdicus ferdicus left a comment

Choose a reason for hiding this comment

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

Thanks

@@ -36,8 +36,14 @@ dependencies {

// Mapbox SDK
if (rootProject.ext.has('rnmbglMapboxLibs')) {
rootProject.ext.get('rnmbglMapboxLibs').split(';').each {
implementation it
def libs = rootProject.ext.get('rnmbglMapboxLibs')
Copy link
Member

Choose a reason for hiding this comment

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

Can't we move rnmbglMapboxLibs and rnmbglMapboxPlugins in some form of constant, you know have one place to change it.

Also, is this the recommended way to have variable dependencies?
Not gonna complain, just feels kinda hacky :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferdicus sorry I'm not really familiar with groovy and/or gradle. This is what I could came up with. I've seen rootProject.ext.get/rootProject.ext.has in other projects (example)

Comment on lines 36 to 43
rpm_spec = {
url: "https://github.com/maplibre/maplibre-gl-native-distribution",
requirement: {
kind: "upToNextMajorVersion",
minimumVersion: "5.11.0"
},
product_name: "Mapbox"
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this here, when we have it in the Podfile, or why have it there if it's here?
I'd assume you want to extract it from the Podfile to make it more user editable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we also support $RNMBGL_Use_SPM = true to use Maplibre from spm with those default params. Not sure if that's desired/or good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should be opinionated towards one?
Configuration is cool and all, however I feel like the install steps are already confusing to some user.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the short term I think this is for advanced users.

Later we can consider defaulting to MapLibre as It doesn't require the access token dance.

@ferdicus
Copy link
Member

@mfazekas , also let me know, when you're done with this - I can assist with the README/ Installation guides/ Changelog and test a bit

@mfazekas
Copy link
Contributor Author

@ferdicus I'm done with the PR

@mfazekas mfazekas merged commit acec80f into master Apr 17, 2021
@ferdicus
Copy link
Member

@ferdicus I'm done with the PR

Cool, thanks for this - I'll try to test this in the coming days. 👍🏿

@jaltin
Copy link
Contributor

jaltin commented Apr 21, 2021

Amazing, thanks for your enormous work on this @mfazekas and @ferdicus!

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.

Switching to MapLibre?
3 participants