-
Notifications
You must be signed in to change notification settings - Fork 904
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
feat: replace {{ packageName }} in deps' configuration #871
Conversation
There was a problem hiding this 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? :)
I should note this will unfortunately break |
There was a problem hiding this 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?
|
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.
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 I have tested the change by manually copying the // 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. 🤔 |
Thanks for making this backwards compat @sjchmiela - LGTM |
There was a problem hiding this 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
The purpose of this PR was to enable a scenario where we don't import the There are scenarios where this would break the build. What this PR does is... rather than importing the As a result, when library author uses FQDN reference themselves, e.g. |
Summary:
Hard importing
${packageName}.R
and${packageName}.BuildConfig
helps to install React Native modules which useR
andBuildConfig
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:
{{ packageName }}
inpackageImportPath
andpackageInstance
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
started the app, and
PackageList.java
containedand