-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
@@ -24,7 +24,7 @@ public MapsPackage() { | |||
return Arrays.<NativeModule>asList(new AirMapModule(reactContext)); | |||
} | |||
|
|||
@Override | |||
// Deprecated RN 0.47 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on this! :)
@lelandrichardson @felipecsl @spikebrehm Thoughts on the best way to move forward? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* '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)
facebook/react-native@ce6fb33