Skip to content

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

Merged
grabbou merged 10 commits into
react-native-community:masterfrom
expo:feat/package-import-path-sub
Mar 19, 2020
Merged

feat: replace {{ packageName }} in deps' configuration#871
grabbou merged 10 commits into
react-native-community:masterfrom
expo:feat/package-import-path-sub

Conversation

@sjchmiela

@sjchmielasjchmiela commented Nov 29, 2019

Copy link
Copy Markdown
Contributor

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-mapsimporthost.exp.exponent.R;
importcom.maps;

and

newMapsPackage(host.exp.exponent.BuildConfig),

@sjchmielasjchmiela changed the title feat: replace {{ packageName }} in deps' packageImportPathfeat: replace {{ packageName }} in deps' configurationNov 29, 2019

@thymikeethymikee left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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 😕).

@SalakarSalakar left a comment

Copy link
Copy Markdown
Member

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?

Comment threadpackages/platform-android/native_modules.gradle Outdated
Comment threadpackages/platform-android/native_modules.gradle Outdated
@sjchmiela

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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

newCodePush(
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

// 1newCodePush(getResources().getString(R.string.CodePushDeploymentKey), getApplicationContext(), BuildConfig.DEBUG)
// 2newPackageR(), newRoyal(), Module.configure(other.package.BuildConfig)

which printed

// 1newCodePush(getResources().getString(com.test.sjchmiela.R.string.CodePushDeploymentKey), getApplicationContext(), com.test.sjchmiela.BuildConfig.DEBUG)
// 2newPackageR(), newRoyal(), 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
Copy Markdown
Member

Thanks for making this backwards compat @sjchmiela - LGTM

Comment threadpackages/platform-android/native_modules.gradle
Comment threadpackages/platform-android/native_modules.gradle

@grabbougrabbou left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

grabbou commented Mar 19, 2020

Copy link
Copy Markdown
Member

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:masterMar 19, 2020
@sjchmiela
sjchmiela deleted the feat/package-import-path-sub branch May 15, 2020 19:50
Sign up for freeto 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

@sjchmiela@grabbou@Salakar@thymikee