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

Swap download scripts for explicit Mapbox-iOS-SDK pod dependency #1533

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mikemorris
Copy link

Switches from an old preinstall script downloading the Mapbox iOS SDK directly from S3 to explicitly listing it as a dependency in the podspec, as well as adding the parent project Frameworks directory to FRAMEWORK_SEARCH_PATHS to support more configurable manual installations.

The motivation for this was avoiding some hiccups with preinstall scripts not running as expected in CI builds in a Lerna project, but I think it's a general improvement, should simplify upgrading the core SDK without waiting on a react-native-mapbox-gl release (for manual installations), and hopefully should help avoid confusing framework not found Mapbox errors like #741 (comment)

@kristfal
Copy link
Collaborator

@mikemorris: I see that you’ve removed the scripts from package.json. Wouldn’t that break the automatic download for anyone who isn’t using pods?

@mikemorris
Copy link
Author

@kristfal It would, but an automatic download of a dependency (instructions don't currently mention where node_modules/@mapbox/react-native-mapbox-gl/ios/Mapbox.framework comes from) feels at odds with the intent of a manual installation process (where you may have a reason to be using a specific version of the SDK, like in a hybrid React Native app).

This would be a breaking build change, so suggesting this for the pending 7.0.0 release.

@kristfal
Copy link
Collaborator

Coming from a “NPM world”, I’d expect native dependencies to be downloaded along with the rest of the lib. Other projects (ex: Detox), follows this path as well. I’m not sure what common practice is on iOS, but I think it makes sense to align with The “NPM way”.

I propose we keep this PR open and wait for 7.0.0 and Nick’s feedback till we make a call.

@mikemorris
Copy link
Author

I propose we keep this PR open and wait for 7.0.0 and Nick’s feedback till we make a call.

👍

My inspiration for this approach was the following React Native libraries that integrate external frameworks:

@kristfal
Copy link
Collaborator

@mikemorris After reviewing a few old tickets and #1535, I'm backtracking on my proposal. Yes, pods makes sense, let's do this! While doing this, could you update the PR to rename the framework to prevent clashes with other Mapbox frameworks (see #1535)? Something like RNMapbox.framework would be suitable I think.

@mattijsf
Copy link
Contributor

I think the current preinstall scripts tries to solve somewhat the fragmented dependency management for iOS projects (manual vs. carthage vs. cocoapods).

I'm Carthage user so I would prefer not to be forced to include a dependency on Cocoapods. As long as there is still a way to manually download + link the mapbox-ios-sdk 👍

@mikemorris
Copy link
Author

mikemorris commented Mar 19, 2019

While doing this, could you update the PR to rename the framework to prevent clashes with other Mapbox frameworks

@1ec5 It appears to me that the issue was "RNMBGL is distributed with it's own copy of Mapbox.framework rather than as (the build product of this library is libRCTMGL.a). Also, hi!

@kristfal Because this removes the automatic download, there should no longer be a separate Mapbox.framework file outside of a developer's control causing issues like #1535. A developer could manually download and add Mapbox-iOS-SDK to the ios/Frameworks directory of a React Native project using whichever name they wish to avoid collisions, or both libraries could potentially link to a single underlying framework dependency.

I think the biggest gotcha with attempting to link to a single framework for both libraries might be semver compatibility, as react-native-mapbox-gl would have a dependency on "Mapbox-iOS-SDK", "~> 3.7.8", while MapboxNavigation depends on "Mapbox-iOS-SDK", "~> 4.3" (I'm not familiar with if/how Cocoapods handles dependency conflicts).

@ClementPF @mysport12 would certainly appreciate if you could test this approach!

@mikemorris
Copy link
Author

As long as there is still a way to manually download + link the mapbox-ios-sdk 👍

@mattijsf Yep, definitely - this PR adds both of the following paths to FRAMEWORK_SEARCH_PATHS

"$(PROJECT_DIR)/../../../ios/Frameworks",
"$(PROJECT_DIR)/../../../ios/Pods",

... so libRCTMGL should be able to find the Mapbox-iOS-SDK framework in either of the following locations, with the first directory being a common place for frameworks to be manually installed.

$(REACT_NATIVE_PROJECT_ROOT)/ios/Frameworks
$(REACT_NATIVE_PROJECT_ROOT)/ios/Pods

Explicit Carthage support wasn't a priority for me as it's currently unsupported by React Native itself, but open to adding another entry to FRAMEWORKS_SEARCH_PATH if there's a default directory used by Carthage that would lessen the need for manual configuration.

@mysport12
Copy link

@mikemorris @kristfal I am up against a few deadlines this month, but can dive in after the 1st :)

@mysport12
Copy link

I agree though that the dependency version mismatch between RNMBGL and Mapbox Nav could cause some issues. For those looking to add in navigation, manual linkage may still be required.

@1ec5
Copy link
Contributor

1ec5 commented Jun 15, 2019

Hi @mikemorris! I haven’t been keeping up with RNMBGL notifications and only now noticed this PR. I think RNMBGL is all caught up with the map SDK as of react-native-mapbox-gl/maps#89, but for what it’s worth:

I agree though that the dependency version mismatch between RNMBGL and Mapbox Nav could cause some issues. For those looking to add in navigation, manual linkage may still be required.

The navigation SDK upgraded to map SDK v4 some time ago; at that time, it started using expressions to style the route line on the map. Style functions (in v3) and expressions (in v4) are mutually incompatible. Since then, the navigation SDK has continued to adopt features that aren’t in map SDK v3, and the next version of the navigation SDK will require map SDK v5.

@mikemorris
Copy link
Author

mikemorris commented Jun 16, 2019

@1ec5 👋 SDK compat stuff makes sense, glad it'll be easier with both on v5 soon 👍

@mfazekas @kristfal would it be worthwhile to close and reopen this against https://github.com/react-native-mapbox-gl/maps? I haven't followed the ownership changes/reorganization around this module too closely.

@kristfal
Copy link
Collaborator

@mikemorris sorry for slow reply. I'll have a look at it via https://github.com/react-native-mapbox-gl/maps/issues/610

Let me know if you think we should change this approach.

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.

5 participants