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

Projects
None yet
7 participants
@declanelcocks
Copy link
Contributor

declanelcocks commented Mar 14, 2019

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 react-native@0.58.0
  • 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') &&

This comment has been minimized.

Copy link
@vovkasm

vovkasm Mar 14, 2019

Contributor

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

This comment has been minimized.

Copy link
@declanelcocks

declanelcocks Mar 14, 2019

Author Contributor

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

@declanelcocks declanelcocks force-pushed the declanelcocks:master branch from 66698f1 to 625eee2 Mar 14, 2019

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 14, 2019

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

@vovkasm

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 14, 2019

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

@vovkasm

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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-community:master Mar 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rborn

This comment has been minimized.

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

This comment has been minimized.

Copy link

forki commented Mar 15, 2019

I can confirm master branch now works for me on android

@kumaresan-cgvak

This comment has been minimized.

Copy link

kumaresan-cgvak commented Mar 15, 2019

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

@christopherdro

This comment has been minimized.

Copy link
Collaborator

christopherdro commented Mar 15, 2019

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

@rborn

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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
You can’t perform that action at this time.