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

[expo] Upgrade react-native-maps to 0.24.2 #4158

Merged
merged 9 commits into from
May 10, 2019

Conversation

sjchmiela
Copy link
Contributor

Just a regular dependency update. 🙂

@sjchmiela sjchmiela requested a review from mczernek May 8, 2019 15:20
Copy link
Contributor

@mczernek mczernek left a comment

Choose a reason for hiding this comment

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

Don't we want to use community version? Are you sure that using our fork is still needed?

@sjchmiela
Copy link
Contributor Author

I updated our fork to match the community master, so now the only difference is:

react-native-maps/react-native-maps@master...expo:master

@mczernek
Copy link
Contributor

mczernek commented May 9, 2019

Yeah, that's why i think we might migrate to community repo - it will require less work to update in the future. But that's just suggestion :)

@sjchmiela
Copy link
Contributor Author

It's the question of whether we want to maintain our fork by git pull --rebaseing every time we want to update the library or whether we want to add a new preprocessor declaration to our build process. Last time we discussed, @tsapeta and I have settled on maintaining the fork. @tsapeta, what do you think now?

@mczernek
Copy link
Contributor

mczernek commented May 9, 2019

It's the question of whether we want to maintain our fork by git pull --rebaseing every time we want to update the library or whether we want to add a new preprocessor declaration to our build process. Last time we discussed, @tsapeta and I have settled on maintaining the fork. @tsapeta, what do you think now?

My suggestion is to go with declaring exact commit (tag) as a parameter of gulp task, which might be overriden by --commit argument. This way we still keep track of used version, prevent ourselves from accidental updates but do not need to have fork when it's not required. What do you think?

@tsapeta
Copy link
Member

tsapeta commented May 10, 2019

I agree that if the fork is not really required, then it would be better to just use the community repo. I don't remember why I removed HAVE_GOOGLE_MAPS flag, but maybe it is worth to check whether we still need it.

@sjchmiela
Copy link
Contributor Author

All the Google Maps classes declarations are ifdef wrapped with this macro, so with this flagged undefined we don't get any react-native-maps Google Maps classes. We have to either define it or remove the macros. 🙂

@sjchmiela
Copy link
Contributor Author

x-ref expo/expo-cli#601

@sjchmiela sjchmiela requested a review from tsapeta May 10, 2019 14:21
CHANGELOG.md Outdated Show resolved Hide resolved
sjchmiela and others added 2 commits May 10, 2019 16:28
Co-Authored-By: Tomasz Sapeta <tsapeta@users.noreply.github.com>
@sjchmiela sjchmiela merged commit 46f48f6 into master May 10, 2019
@sjchmiela sjchmiela deleted the @sjchmiela/upgrade_react_native_maps branch May 10, 2019 14:41
sjchmiela added a commit to expo/expo-cli that referenced this pull request May 13, 2019
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.

3 participants