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 autolink script #5990

Merged
merged 5 commits into from Mar 3, 2020
Merged

Fix autolink script #5990

merged 5 commits into from Mar 3, 2020

Conversation

jinshin1013
Copy link
Contributor

This PR fixes the autolink script including:

@guyca I've assumed the minSdkVersion will be 19 unless user specified higher. I've got the version from the doc, should this be updated to something higher?

@jinshin1013 jinshin1013 mentioned this pull request Mar 3, 2020
Copy link
Collaborator

@guyca guyca left a comment

Choose a reason for hiding this comment

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

Hey @jinshin1013 Thanks for the PR!

  • minSdk should be 19 as we don't support 18 and below. At Wix, our minSdk is 21 but it really depends on your needs. If you don't test your app against older devices and can't guarantee it performs as expected on these old devices then you better increase your minSdk to 21.

  • Can you please explain why the change to podfile is needed? Is it to remove the pod definition in case users added it by following previous installation instructions?

autolink/postlink/gradleLinker.js Outdated Show resolved Hide resolved
autolink/postlink/gradleLinker.js Outdated Show resolved Hide resolved
@jinshin1013
Copy link
Contributor Author

jinshin1013 commented Mar 3, 2020

@guyca Thanks for catching the regex issue, I'm still trying to get myself used to it 😓

minSdk should be 19 as we don't support 18 and below.

Makes sense for minSdkVersion to be at least 19 and I also reckon most people would use 21 anyway.

Can you please explain why the change to podfile is needed?

I've found that whenever I run react-native link react-native-navigation it adds the pod definition for react-native-navigation. I'm using @react-native-community/cli@3.2.0 so I don't think it's the cli problem. You should be able to reproduce this problem with fresh react-native project. Also as you mentioned I reckon it'd be good for the existing users who followed the previous installation step anyway.

@guyca
Copy link
Collaborator

guyca commented Mar 3, 2020

I've found that whenever I run react-native link react-native-navigation it adds the pod definition for react-native-navigation.

Isn't this desired? I thought react-native link adds all native dependencies to the podfile.

@jinshin1013
Copy link
Contributor Author

jinshin1013 commented Mar 3, 2020

Isn't this desired? I thought react-native link adds all native dependencies to the podfile.

That is true for react-native version below 0.60 as they do not support autolinking. But I believe tools should assume users are on the "new" react-native and add somewhere in the documentation for the legacy linking (adding Pod definition). Thoughts?

@jinshin1013 jinshin1013 requested a review from guyca March 3, 2020 12:32
@guyca guyca merged commit 4ce0e89 into wix:master Mar 3, 2020
@jinshin1013 jinshin1013 deleted the fix-autolink branch March 3, 2020 21:00
stachu2k pushed a commit to stachu2k/react-native-navigation that referenced this pull request Apr 8, 2020
This PR fixes the autolink script including:
* Specifying the minSdkVersion for Android. Closes wix#5983.
* Removing the RNN Pod added by the react-native link script.
stachu2k pushed a commit to stachu2k/react-native-navigation that referenced this pull request Apr 8, 2020
stachu2k pushed a commit to stachu2k/react-native-navigation that referenced this pull request Apr 8, 2020
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.

RN CLI Autolinking error
2 participants