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

feat: replace {{ packageName }} in deps' configuration #871

Merged

Conversation

sjchmiela
Copy link
Contributor

@sjchmiela sjchmiela commented Nov 29, 2019

Summary:

Hard importing ${packageName}.R and ${packageName}.BuildConfig helps to install React Native modules which use R and BuildConfig without package prefix (eg. react-native-code-push, see #435 fixing #434.) BuildConfig import has been introduced in #258, so right from the beginning (specific commit: 64f3f5a).

This pull request:

  • adds support for interpolating {{ packageName }} in packageImportPath and packageInstance
  • adds support for generatePackageList task to libraries (imagine a project where its the submodule (:app depends on ➡️ :library) which depends on autolinked React Native modules)

Test Plan:

I introduced these changes to my project, added to react-native-maps' react-native.config.js

 module.exports = {
   dependency: {
     platforms: {
       android: {
         sourceDir: './lib/android',
+        packageInstance: 'new MapsPackage({{ packageName }}.BuildConfig)',
+        packageImportPath: 'import {{ packageName }}.R;\nimport com.maps;'
       },
     },
   },
 };

started the app, and PackageList.java contained

// react-native-maps
import host.exp.exponent.R;
import com.maps;

and

new MapsPackage(host.exp.exponent.BuildConfig),

@sjchmiela sjchmiela changed the title feat: replace {{ packageName }} in deps' packageImportPath feat: replace {{ packageName }} in deps' configuration Nov 29, 2019
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Sounds good! Can we document it? :)

@sjchmiela
Copy link
Contributor Author

I should note this will unfortunately break react-native-code-push installations until it adds those value placeholders to the config (or users are provided with a patch-package solution 😕).

Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

Can we not keep this backwards compatible for libraries such as CodePush that are using BuildConfig? I think we should if we can?

I'm not entirely sure why import ${packageName}.BuildConfig needs removing when you're adding it back via your module config?

packages/platform-android/native_modules.gradle Outdated Show resolved Hide resolved
@sjchmiela
Copy link
Contributor Author

packageName gets resolved to packageName of :app (let's say com.app) via react-native config, whereas :library has another package, eg. com.library. Adding import com.app.R in :library wouldn't work (without adding a dependency on :app, which is something I wouldn't need/shouldn't be required to do).

@grabbou
Copy link
Member

grabbou commented Jan 30, 2020

This is great - @sjchmiela - what is the status of it? Is this good to go? One of the previous comments mentions that this is potentially going to break CodePush. Was this already addressed?

Libraries already expect to be able to use R and BuildConfig
classes without having to worry about their packages. To match the
expectations the script will replace un-fully-qualified references
to fully-qualified.
@sjchmiela
Copy link
Contributor Author

No, it wasn't up until now! I have brought back support for eg.

new CodePush(
    getResources().getString(R.string.CodePushDeploymentKey),
    getApplicationContext(),
    BuildConfig.DEBUG
)

by a middle step of replacing R with {{ packageName }}.R and BuildConfig with {{ packageName }}.BuildConfig.

I have tested the change by manually copying the interpolateDynamicValues function to a Gradle file and executing it on

// 1
new CodePush(getResources().getString(R.string.CodePushDeploymentKey), getApplicationContext(), BuildConfig.DEBUG)
// 2
new PackageR(), new Royal(), Module.configure(other.package.BuildConfig)

which printed

// 1
new CodePush(getResources().getString(com.test.sjchmiela.R.string.CodePushDeploymentKey), getApplicationContext(), com.test.sjchmiela.BuildConfig.DEBUG)
// 2
new PackageR(), new Royal(), Module.configure(other.package.BuildConfig)

Let me know if this will be the proper way to go in your opinion.

PS. I think the test failures weren't caused by this change. 🤔

@Salakar
Copy link
Member

Salakar commented Feb 3, 2020

Thanks for making this backwards compat @sjchmiela - LGTM

Copy link
Member

@grabbou grabbou left a comment

Choose a reason for hiding this comment

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

Code-wise it all looks good - thank you! Just left a question to better understand it

@grabbou
Copy link
Member

grabbou commented Mar 19, 2020

I am not sure if this PR will fix #938. The issue that #938 is having is due to multiple apps - CLI currently doesn't support an array of apps and will always use the first one.

@grabbou
Copy link
Member

grabbou commented Mar 19, 2020

The purpose of this PR was to enable a scenario where we don't import the import {{packageName}}.R; and import {{packageName}}.BuildConfig to the scope of the file.

There are scenarios where this would break the build.

What this PR does is... rather than importing the BuildConfig to the scope, it will replace its reference from BuildConfig to {{packageName}}.BuildConfig directly at the call site.

As a result, when library author uses FQDN reference themselves, e.g. com.mycustompackage.BuildConfig rather than BuildConfig, we will not refer to MainApplication packageName (e.g. com.acme.BuildConfig) from that file, making it work.

@grabbou grabbou merged commit c98214a into react-native-community:master Mar 19, 2020
@sjchmiela sjchmiela deleted the feat/package-import-path-sub branch May 15, 2020 19:50
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.

4 participants