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

Upgrade React Native peer dependency to 0.54. #2115

Merged
merged 5 commits into from
Mar 27, 2018

Conversation

lachenmayer
Copy link
Contributor

Does any other open PR do the same thing?

No

What issue is this PR fixing?

This library has some regressions when used with React Native 0.54 - we were using the project with 0.54, and were facing #2100 for example.

Further, I was trying set up some iOS integration tests using the built-in RCTTest library, and I was not able to set it up using RN 0.51. (It works fine on 0.54)

It would be great if we could upgrade the RN version so that this work can continue. I feel that native<->JS integration tests (& possibly also native snapshot tests) will make it much easier to maintain backwards compatibility & avoid serious regressions like the current one we're facing.

How did you test this PR?

  1. check out the PR branch
  2. yarn / npm install
  3. yarn build:ios / npm run build:ios
  4. Open example/ios/AirMapsExplorer.xcworkspace (make sure it's the workspace, not the project)
  5. Run

I have not tested this change with Android, as I currently don't have Android Studio installed. I have updated the Android manifest file as per rn-diff, so in theory this should build properly. I would appreciate any help with that - a simple npm install && npm run:android should do the trick.

Known issues

  1. It is trivial to trigger GoogleMaps iOS: initialRegion latitudeDelta/longitudeDelta not respected #2100 now - just open the example app, switch to Google Maps and open any of the examples. Most examples should be zoomed into SF, but instead you see all of North America. If this PR is going to be merged, I would merge GoogleMaps iOS: initialRegion latitudeDelta/longitudeDelta not respected #2100 along with it.

  2. The Podfile currently does not work if you are using an old version of RN. This shouldn't be too difficult to fix, I will update this PR with a fix & documentation. The old Podfile was relying on the deprecated BatchedBridge, I think it should be possible to use CxxBridge in any RN versions we care about.

Changes to Bundler

This PR also makes the following changes to how Ruby gems are installed:

  1. Remove the bundler "binstubs" which were previously in examples/ios.

    Binstubs exist so that Ruby "binaries" can be executed without using bundle exec. See here for an explanation.

    The only place bundles are used currently is in the build:ios script in package.json. This already uses bundle exec, so the binstubs were actually never used.

  2. Install the Rubygem bundles locally in example/ios/bundles.

    bundler previously installed the dependencies in the global gems directory. If you are using the default macOS Ruby installation, this requires sudo. Since we already install node_modules & pods within the repo, I believe it makes sense to also install these locally.

  3. Update the Gemfile.lock.

    The current Gemfile.lock specified an outdated version of Cocoapods. This throws a warning on the current example Podfile. Cocoapods is now updated to version 1.4.0.

The Podfile is updated in line with the required React Native dependency changes.

This commit also makes the following changes to how Ruby gems are installed:

1. Remove the bundler "binstubs" which were previously in `examples/ios`.

    Binstubs exist so that Ruby "binaries" can be executed without using
    `bundle exec`. See here for an explanation:
    https://github.com/rbenv/rbenv/wiki/Understanding-binstubs#project-specific-binstubs

    The only place bundles are used currently is in the `build:ios` script in
    `package.json`. This already uses `bundle exec`, so the binstubs were
    actually never used.

2. Install the Rubygem bundles locally in `example/ios/bundles`.

    `bundler` previously installed the dependencies in the global gems directory.
    If you are using the default macOS Ruby installation, this requires `sudo`.

    Since we already install node_modules & pods within the repo, I believe it
    makes sense to also install these locally.

3. Update the `Gemfile.lock`.

    The current `Gemfile.lock` specified an outdated version of Cocoapods.
    This throws a warning on the current example Podfile.
    Cocoapods is now updated to version 1.4.0.
@willbattel
Copy link
Contributor

Any ETA on merging this?

@rborn
Copy link
Collaborator

rborn commented Mar 26, 2018

@wbattel4607 did you test by any chance this PR?

@willbattel
Copy link
Contributor

@rborn I haven't had any issues with it yet.

@rborn
Copy link
Collaborator

rborn commented Mar 26, 2018

@wbattel4607 @lachenmayer the only issue I see here is backward compat for RN < 0.54

@alvelig thoughts on this?

@willbattel
Copy link
Contributor

willbattel commented Mar 26, 2018

The README suggests that this repo will only support the latest version of React Native- though I understand that doesn't mean pulling the plug on everyone else is the right thing to do. Perhaps it could get bundled with a new minor release version, where those on RN < 0.54 can stick to 0.20.x and you can publish any other changes as patch versions on 0.20.

Speaking of semver, when will the first major version be? Do you have any plans for a 1.0.0 release? I think the lib is ready for it but I don't know if the collaborators have any further plans before then.

If you were to release version 1.0.0 as the first major version, you could easily just say that version 1.X.X required RN >= 0.54 and have 0.X.X remain compatible with RN < 0.54. You can still publish more minor and patch versions to 0.X.X as fixes, optimizations, etc, and just go forward with 1.X.X for new features, etc.... This is what I would do.

@lachenmayer
Copy link
Contributor Author

Hi @rborn, thanks a lot for checking this out! :)
I have now tested this with RN 0.51-0.53, and the only change that has to be made is to rename the glog pod to GLog. I have added a note in the installation docs.

@rborn
Copy link
Collaborator

rborn commented Mar 27, 2018

@alvelig 🐽

<uses-permission android:name="android.permission.INTERNET" />
<uses-permission android:name="android.permission.ACCESS_COURSE_LOCATION"/>
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />

<application
android:allowBackup="true"
android:allowBackup="false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know - this change was made in RN 0.54 ncuillery/rn-diff@rn-0.53.3...rn-0.54.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, It's not a problem for example

Copy link

@Doko-Demo-Doa Doko-Demo-Doa Mar 27, 2018

Choose a reason for hiding this comment

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

That's just some code cleanups from RN team, nothing serious.

@alvelig
Copy link
Contributor

alvelig commented Mar 27, 2018

LGTM

@rborn rborn merged commit 39d48b1 into react-native-maps:master Mar 27, 2018
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