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

Handle Android RN 0.47 breaking change #1481

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

Benjamin-Dobell
Copy link
Contributor

@@ -24,7 +24,7 @@ public MapsPackage() {
return Arrays.<NativeModule>asList(new AirMapModule(reactContext));
}

@Override
// Deprecated RN 0.47

Choose a reason for hiding this comment

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

I would remove the @Override part instead of the comment. Since comments get old very fast

Copy link
Contributor Author

@Benjamin-Dobell Benjamin-Dobell Jul 17, 2017

Choose a reason for hiding this comment

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

@henrikra I believe the comment is important as it explains why this code is here at all. It's actually 100% dead code in RN >= 0.47.

So what this comment means is:

Feel free to use this in your own projects, but the method is deprecated as of React Native 0.47.

Now I could have used a Java @Deprecated annotation, but I believe that's actually misleading in this circumstance. @Deprecated is designed to provide warnings to end-users that they're calling/using code that they shouldn't be using anymore. However, if you're still using RN < 0.47 this method is required, there is no alternative. You don't want compiler warnings complaining you're using a deprecated method.

The real alternative to this pull request is not just to delete @Override, as that's confusing and leaves around dead code without an explanation; the alternative is to simply delete the method entirely. However, that's problematic as it would mean users of react-native-maps can't update (obtain fixes/patches) unless they also upgrade React Native to >= 0.47, which depending on the project may be a large undertaking.

Thus I believe a succinct comment is the best option.

Choose a reason for hiding this comment

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

I agree on this! :)

@christopherdro
Copy link
Collaborator

@lelandrichardson @felipecsl @spikebrehm Thoughts on the best way to move forward?

Copy link
Contributor

@felipecsl felipecsl left a comment

Choose a reason for hiding this comment

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

LGTM!

@felipecsl felipecsl merged commit 1db4fff into react-native-maps:master Jul 18, 2017
sorodrigo pushed a commit to Vizzuality/react-native-maps that referenced this pull request Aug 21, 2017
* 'master' of https://github.com/airbnb/react-native-maps:
  v0.16.2
  Revert "Issue1176 improve ios marker performance by X100 (react-native-maps#1187)"
  Fix initial region android (react-native-maps#1563)
  v0.16.1
  Enhance Podfile. (react-native-maps#1252)
  Update marker component (react-native-maps#1428)
  Add legalNotice constant (react-native-maps#1458)
  Issue1176 improve ios marker performance by X100 (react-native-maps#1187)
  Fix initial region native prop (react-native-maps#1546)
  fix `Archive` configuration for iOS builds (react-native-maps#1550)
  v0.16.0
  Document MapView min/max zoom properties (react-native-maps#1538)
  Fix timing function used in AnimatedRegion.spring (react-native-maps#1479)
  Fix crashing the application when a user presses on the map and the Google Play Services need to be updated or at the moment of the process of updating (react-native-maps#1469)
  skip region monitoring if map object is null (react-native-maps#1443)
  Zoom level fixes (react-native-maps#1485)
  Attempt to fix crashes. A variant of react-native-maps#1403 but for another lifecycle method, as proposed by @Nelrohd. (react-native-maps#1464)
  Handle Android RN 0.47 breaking change (react-native-maps#1481)
  add MKTileOverlayRenderer (react-native-maps#1357)
  Add onMapReady callback (react-native-maps#1369)
pjaraherrera pushed a commit to pjaraherrera/react-native-maps that referenced this pull request Sep 27, 2017
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.

None yet

4 participants