Fix android duplicate linking #18131
Conversation
android isInstalled has a different signature than ios counterpart
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
| @@ -49,7 +49,8 @@ const linkDependency = (platforms, project, dependency) => { | |||
| return null; | |||
| } | |||
|
|
|||
| const isInstalled = linkConfig.isInstalled(project[platform], dependency.config[platform]); | |||
| const isInstalledParameter = platform == "android" ? dependency.name : dependency.config[platform]; | |||
| const isInstalled = linkConfig.isInstalled(project[platform], isInstalledParameter); | |||
There was a problem hiding this comment.
That makes sense, although I would be much more in favor of having it use the same signature.
CC: @Kureev
There was a problem hiding this comment.
The thing is dependency.config[platform] does not contain dependency.name, should we add dependencyName to dependency.config[platform] object then?
There was a problem hiding this comment.
Unlink suffers the same issue for android and iOS unlink is broken
There was a problem hiding this comment.
Let's just add the name parameter to the isInstalled signature.
|
Can you please elaborate why we need an exception for Android? I'd also prefer to keep signature the same. |
|
@Kureev the object ios uses, I debugged it and does not contain the Also when refactoring was made on this code (not sure which version but it's after 0.48) it just broke android. Android isInstalled requires a single name string but the call was changed to pass a config object that only ios version understands and not the android one. Thus the isInstalled android version always returns false. |
|
Seems to be this commit: a40bfa7#diff-ca7cc59db13ac8eb27fe0a9a6c9debee |
facebook-github-bot
added
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@Kureev @grabbou any suggestion for going forward? This impacts every single library having native android code. Without this patch every single time you run This patch fixes the regression when the code was refactored in some recent version of react-native. |
|
Been hit with this bug.. no matter what react-native version.. new create and old also effected. |
|
I've pinged @Kureev to look into it - let's wait for a day or two and then, I'll cherry-pick this back to the releases that are affected. I am really sorry for any inconvenience. |
|
I am even more sorry for this. This is absolutely not @grabbou's fault, I take all the blame for this one. I had mixed up two branches where I was working on support for linking platforms other than iOS and Android and ended up testing the wrong code. Very sorry @grabbou and @Kureev that we have to patch those releases again. 👎 |
;)) |
android isInstalled has a different signature than ios counterpart
Motivation
Run
react-native linkon a project with an android library that is already linked: the check will fail and will link again.This should fix #18053
Test Plan
AppCenter SDK for React-Native uses linking. Running react-native link on appcenter package would constantly add the imports, the moduleConfig strings and add duplicated code to MainApplication.java without this patch.
I also tested it didn't break ios linking.
Release Notes
[ANDROID] [BUGFIX] Fix
react-native linkadding duplicate code and configuration when running a second time.