Add support for scoped npm package linking in local-cli

[vekerdyb]

Motivation

Linking scoped npm packages breaks the android build using the local cli.

I found no documentation for this, but Android Studio correctly replaces the / in the npm package name with _ in the gradle include statement.

Test Plan

The test included with this PR verifies, that scoped dependencies, are linked with this replacement.

Release Notes

[CLI] [BUGFIX] [local-cli/link/link.js] - Change link function for scoped npm packages to link as expected on Android

[pull-bot]

Attention: @grabbou, @Kureev

Generated by 🚫 dangerJS

[facebook-github-bot]

@vekerdyb I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[Kureev]

Looks good to me! 🚀

[grabbou]

Thanks for submitting this PR. I left few comments. Let me know if you need any help.

[grabbou]

I don't think this is the right place for this change. First and foremost thing is that it mutates dependency object making this function no longer pure.

Another thing is that I can't tell where the new value with / replaced to _ should be used. In your comment you said that it breaks include statement in Gradle.

Maybe you should do this replacement directly inside the function that puts include statement into Gradle file?

[grabbou]

E.g. here https://github.com/vekerdyb/react-native/blob/e9e841628fc77faa9bd271c86a031d0518d5e7e6/local-cli/link/android/patches/makeSettingsPatch.js#L22, make const escapedName = name.replace() and done :) (hopefully)

[Kureev]

Mmm, that will only work if this name isn't re-used anywhere else. IIRC, we use it in many places, so we need to have a consistent name across different functions. As a compromise, we can introduce a new function, something like sanitizeName or similar that only transforms a given dependency name into a version that will be understandable by Grade and other parts of the react-native link.

[grabbou]

Mmm, that will only work if this name isn't re-used anywhere else

Yup, and that way it's explicit where we sanitize the name. Doing it globally on a dependency can have side effects as the name field is supposed to be the name of the package.

Function is a great idea that plays well with our design. I think I've seen it already in generator subfolder where they do the same for react-native init (sanitize the name when creating Android main application). We could potentially reuse that as well :)

[vekerdyb]

Thanks for the feedback! Unfortunately I am quite busy at the moment, but will get back to this mid February, unless it is picked up by someone else by then.

I think I've seen it already in generator subfolder where they do the same for react-native init (sanitize the name when creating Android main application). We could potentially reuse that as well

I agree, I faintly remember seeing init working as expected now.

[rozele]

I published #18207 to fix the issue for Android and iOS.

[Titozzz]

Thanks for the fix for android, but you also fixed iOS at the same time by importing the correct file ? Looks like it @rozele

[Titozzz]

I'll test your fix and make mine from your branch if this works

[Titozzz]

Because your branch is being landed, i'll apply my fixes to it 💛

[Titozzz]

There you go: #18275

[Titozzz]

@Kureev You can close this issue now that #18275 has landed

[cpojer]

Thank you for your PR. We just moved this code to https://github.com/react-native-community/react-native-cli so please submit your PR there.