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

Fix deprecated UIManager usage when accessing component names #2740

Merged
merged 3 commits into from
Mar 15, 2019

Conversation

declanelcocks
Copy link
Contributor

Does any other open PR do the same thing?

https://github.com/react-native-community/react-native-maps/pull/2671/files

What issue is this PR fixing?

As mentioned in #2620, there are some YellowBox warnings with deprecated UIManager calls being used. The PR above fixed some of the warnings, but a warning still pops up regarding AIRMapLite and AIRMap.

How did you test this PR?

  • Updated iOS application that uses react-native-maps to [email protected]
  • Open screen with any MapView component
  • Check that no warnings regarding deprecated UIManager usage exists

@@ -915,7 +937,7 @@ if (Platform.OS === 'android') {
}
const getAirMapComponent = provider => airMaps[provider || 'default'];

const AIRMapLite = NativeModules.UIManager.AIRMapLite &&
const AIRMapLite = NativeModules.UIManager.getViewManagerConfig('AIRMapLite') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably will crash on RN <0.58 because UIManager does not have property getViewManagerConfig. Right?

Copy link
Contributor Author

@declanelcocks declanelcocks Mar 14, 2019

Choose a reason for hiding this comment

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

@vovkasm Good point, I just pushed a commit to add the same check for UIManager.getViewManagerConfig here too.

@rborn
Copy link
Collaborator

rborn commented Mar 14, 2019

@vovkasm please let us know if the latest change it's ok and we merge 🎉

@vovkasm
Copy link
Contributor

vovkasm commented Mar 14, 2019

@rborn technically it looks correct. But I would recommend refactor the code to have logic in one place and not to spread hacks through source files.

@rborn
Copy link
Collaborator

rborn commented Mar 14, 2019

@vovkasm @declanelcocks can you two please coordinate, maybe we get something better out of this 🤗

@vovkasm
Copy link
Contributor

vovkasm commented Mar 14, 2019

Ahm... Unfortunately I'm currently almost has no time to work on code.
@declanelcocks Can you remove duplication of check logic from project, perhaps make one function to get view config from UIManager? If not, I think PR can be commited as is, because working code with duplication anyway better than no code :-)

@declanelcocks
Copy link
Contributor Author

declanelcocks commented Mar 15, 2019

@vovkasm I can help to refactor it into a helper function, but probably won't get around to it until this weekend or early next week. I'd suggest the same as you and to merge this PR in for now, then make a new PR to refactor the UIManager logic. Seems like they're are several people popping up with this issue so would be cool to get the working code in first as you said.

@forki
Copy link

forki commented Mar 15, 2019

can you please merge as is and refactor later? this one is really important and fixes the map for RN 0.59.x

@rborn
Copy link
Collaborator

rborn commented Mar 15, 2019

@declanelcocks @vovkasm I'll merge this one, please ping me when you refactor the code 🤗

@rborn rborn merged commit 2adc143 into react-native-maps:master Mar 15, 2019
@rborn
Copy link
Collaborator

rborn commented Mar 15, 2019

@christopherdro maybe we should make a release so we have RN 0.59 working form npm?

@forki
Copy link

forki commented Mar 15, 2019

I can confirm master branch now works for me on android

@kumaresan-cgvak
Copy link

When can we expect a new release with the fix for RN 0.59?

@christopherdro
Copy link
Collaborator

We need to put together the release notes.
I can try to do this weekend unless someone else wants to volunteer.

@rborn
Copy link
Collaborator

rborn commented Mar 15, 2019

@forki @kumaresan-cgvak any of you fancy a changelog PR fort he release ?

this is the last published commit 51ff723

and this is an example of a previous changelog: https://github.com/react-native-community/react-native-maps/pull/2666/files#diff-4ac32a78649ca5bdd8e0ba38b7006a1e

Thanks!

@LuiisFernando
Copy link

LuiisFernando commented Apr 1, 2019

How can I install master ?
using the command npm install react-native-maps --save it install version 0.23.0
And If am I correct the fix of this bug is a new version released march 16, right ?
How could I install this version to solve the yellow warning ??

Thanks!

@rborn
Copy link
Collaborator

rborn commented Apr 1, 2019

@LuiisFernando npm install --save react-native-community/react-native-maps should work or npm install --save https://github.com/react-native-community/react-native-maps.git

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.

7 participants