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

Projects
None yet
5 participants
@lachenmayer
Copy link
Contributor

commented Mar 20, 2018

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 #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 #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.

lachenmayer added some commits Mar 20, 2018

Upgrade React Native dev dependency to 0.54.2.
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

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2018

Any ETA on merging this?

@rborn

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2018

@wbattel4607 did you test by any chance this PR?

@willbattel

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

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

@rborn

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2018

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

@alvelig thoughts on this?

@willbattel

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

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

This comment has been minimized.

Copy link
Collaborator

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"

This comment has been minimized.

Copy link
@alvelig

alvelig Mar 27, 2018

Collaborator

Why this?

This comment has been minimized.

Copy link
@lachenmayer

lachenmayer Mar 27, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@alvelig

alvelig Mar 27, 2018

Collaborator

Ok, It's not a problem for example

This comment has been minimized.

Copy link
@Doko-Demo-Doa

Doko-Demo-Doa Mar 27, 2018

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

@alvelig

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2018

LGTM

@rborn rborn merged commit 39d48b1 into react-native-community:master Mar 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.