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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Deprecate MapView in favor of airbnb/react-native-maps #10500

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@mkonicek
Contributor

mkonicek commented Oct 21, 2016

Compared to the <MapView> that comes with React Native, react-native-maps work on Android and is more feature complete. It is actively maintained and used extensively (9.2k installs / month, see JS.Coach).

We think now is a good time to switch to react-native-maps in your applications and make react-native-maps the official <MapView> implementation for React Native.

We are going to release the deprecated <MapView> as a separate npm module so you can migrate to react-native-maps at your own pace.

Test Plan

Checked the docs render correctly on the website:

cd website
npm install
npm start

screenshot 2016-11-01 20 17 31

Warning is shown:

screenshot 2016-11-01 20 39 21

Published the module separately (https://github.com/facebookarchive/react-native-deprecated-modules, will also publish to npm in January) so people can continue using it and migrate to Airbnb maps at their own pace. Linked the module to a sample app using react-native link, it works 馃帀

import MapView from 'deprecated-react-native-ios-mapview';

export default class MapsTest extends Component {
  render() {
    return (
      <View style={styles.container}>
        <Text style={styles.welcome}>
          Welcome to React Native!
        </Text>
        <MapView
          style={{width: 200, height: 200, margin: 40}}
          showsUserLocation={true}
          onRegionChangeComplete={(region) => {alert(region.longitude)}}
        />
...

screenshot 2016-11-01 18 43 14

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 21, 2016

By analyzing the blame information on this pull request, we identified @caabernathy and @spikebrehm to be potential reviewers.

facebook-github-bot commented Oct 21, 2016

By analyzing the blame information on this pull request, we identified @caabernathy and @spikebrehm to be potential reviewers.

@spikebrehm

This comment has been minimized.

Show comment
Hide comment
@spikebrehm

spikebrehm Oct 21, 2016

Contributor

Exciting!

Contributor

spikebrehm commented Oct 21, 2016

Exciting!

@lacker

This comment has been minimized.

Show comment
Hide comment
@lacker

lacker Oct 21, 2016

Contributor

How about making it so that if you use this class, a warning gets logged?

Contributor

lacker commented Oct 21, 2016

How about making it so that if you use this class, a warning gets logged?

@lacker

This comment has been minimized.

Show comment
Hide comment
@lacker

lacker Nov 1, 2016

Contributor

What do people have to do to make this warning go away - just npm install deprecated-react-native-ios-mapview and add the appropriate import? Also, let's not refer to it as "Airbnb maps" any more than we refer to the existing one as "Facebook maps" - let's just call it "the react-native-maps module".

Contributor

lacker commented Nov 1, 2016

What do people have to do to make this warning go away - just npm install deprecated-react-native-ios-mapview and add the appropriate import? Also, let's not refer to it as "Airbnb maps" any more than we refer to the existing one as "Facebook maps" - let's just call it "the react-native-maps module".

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 1, 2016

Contributor

What do people have to do to make this warning go away - just npm install deprecated-react-native-ios-mapview and add the appropriate import?

Yes that will work, but probably only at version >= 0.42 where we remove the native code. If people install deprecated-react-native-ios-mapview (and react-native link it) they'd have the Obj-C code twice, until we remove it in v0.42. Let me check how that works but seems strange.

Ideally people would migrate to react-native-maps to make the warning go away. But I see your point the warning can be annoying and you'd just like to make it go away. We have other deprecation warnings like this in the codebase though - maybe their point is to always be visible until you fix the code?

Contributor

mkonicek commented Nov 1, 2016

What do people have to do to make this warning go away - just npm install deprecated-react-native-ios-mapview and add the appropriate import?

Yes that will work, but probably only at version >= 0.42 where we remove the native code. If people install deprecated-react-native-ios-mapview (and react-native link it) they'd have the Obj-C code twice, until we remove it in v0.42. Let me check how that works but seems strange.

Ideally people would migrate to react-native-maps to make the warning go away. But I see your point the warning can be annoying and you'd just like to make it go away. We have other deprecation warnings like this in the codebase though - maybe their point is to always be visible until you fix the code?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 1, 2016

Contributor

Updated the warning message 馃憤

Contributor

mkonicek commented Nov 1, 2016

Updated the warning message 馃憤

@lacker

This comment has been minimized.

Show comment
Hide comment
@lacker

lacker Nov 2, 2016

Contributor

Looks good. Not sure if the travis error is spurious - I'll try rebuilding. Once CI is clear let's shipit.

Contributor

lacker commented Nov 2, 2016

Looks good. Not sure if the travis error is spurious - I'll try rebuilding. Once CI is clear let's shipit.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek
Contributor

mkonicek commented Nov 2, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 2, 2016

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented Nov 2, 2016

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 2, 2016

Contributor

@lacker Sandcastle confirms this breaks some screenshot tests (probably by showing the warning :)). I'll fix that.

Contributor

mkonicek commented Nov 2, 2016

@lacker Sandcastle confirms this breaks some screenshot tests (probably by showing the warning :)). I'll fix that.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 3, 2016

Contributor

@lacker Should be good to go now, tests passed.

Contributor

mkonicek commented Nov 3, 2016

@lacker Should be good to go now, tests passed.

@lacker

This comment has been minimized.

Show comment
Hide comment
@lacker
Contributor

lacker commented Nov 3, 2016

mkonicek added a commit that referenced this pull request Dec 13, 2016

Deprecate MapView in favor of airbnb/react-native-maps
Summary:
Compared to the `<MapView>` that comes with React Native, [react-native-maps](https://github.com/airbnb/react-native-maps) work on Android and is more feature complete. It is actively maintained and used extensively (9.2k installs / month, see [JS.Coach](https://js.coach/react-native/react-native-maps?search=react-native-maps)).

We think now is a good time to switch to react-native-maps in your applications and make `react-native-maps` the official `<MapView>` implementation for React Native.

We are going to release the deprecated `<MapView>` as a separate npm module so you can migrate to `react-native-maps` at your own pace.

**Test Plan**

Checked the docs render correctly on the website:

```
cd website
npm install
npm start
```

<img width="696" alt="screenshot 2016-11-01 20 17 31" src="https://cloud.githubusercontent.com/assets/346214/19905831/480074b8-a070-11e6-8779-8e12343c2883.png">

Warning is shown:

<img width="423" alt="screenshot 2016-11-01 20 39 21" src="https://cloud.githu
Closes #10500

Differential Revision: D4119602

Pulled By: mkonicek

fbshipit-source-id: 86780a98bf999e6047565ab66a5ebbd15e499a46

DanielMSchmidt added a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017

Deprecate MapView in favor of airbnb/react-native-maps
Summary:
Compared to the `<MapView>` that comes with React Native, [react-native-maps](https://github.com/airbnb/react-native-maps) work on Android and is more feature complete. It is actively maintained and used extensively (9.2k installs / month, see [JS.Coach](https://js.coach/react-native/react-native-maps?search=react-native-maps)).

We think now is a good time to switch to react-native-maps in your applications and make `react-native-maps` the official `<MapView>` implementation for React Native.

We are going to release the deprecated `<MapView>` as a separate npm module so you can migrate to `react-native-maps` at your own pace.

**Test Plan**

Checked the docs render correctly on the website:

```
cd website
npm install
npm start
```

<img width="696" alt="screenshot 2016-11-01 20 17 31" src="https://cloud.githubusercontent.com/assets/346214/19905831/480074b8-a070-11e6-8779-8e12343c2883.png">

Warning is shown:

<img width="423" alt="screenshot 2016-11-01 20 39 21" src="https://cloud.githu
Closes facebook#10500

Differential Revision: D4119602

Pulled By: mkonicek

fbshipit-source-id: 86780a98bf999e6047565ab66a5ebbd15e499a46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment