-
-
Notifications
You must be signed in to change notification settings - Fork 644
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: add expo config plugin #2218
Conversation
5256a2b
to
3da0250
Compare
3da0250
to
bf5a080
Compare
compileSdkVersion rootProject.ext.compileSdkVersion | ||
|
||
defaultConfig { | ||
missingDimensionStrategy "store", "play" |
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.
Looks like these variables have some duplication, any thoughts on using a common template?
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.
Agree that there is too much of it. In current unit tests implementation, we are passing initial build.gradle
(part of it to be precise) to the function and comparing the result with the correct version. This is done 3 times, one for each paymentProvider
. We could remove irrelevant lines and leave only lines close to defaultConfig
. What do you think?
versionCode 34 | ||
versionName "1.16.2" |
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.
Are these arbitrary/ do they matter to the person building an expo app?
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.
All files in the fixtures
directory are used solely for testing and will only be utilized by a developer writing tests.
} | ||
|
||
defaultConfig { | ||
applicationId 'com.test.withIAP' |
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.
should these be customizable?
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.
This is only a part of some faked build.gradle
file to ensure that properties are added correctly in tests. Content is irrelevant. Maybe we should add a comment that explains it?
Thank you for your contribution @pafry7. I added some simple comments/questions. Let me know what you think. Overall great work, thank you for the unit tests too |
|
||
const pkg = require('../../package.json'); | ||
|
||
type PaymentProvider = 'Amazon AppStore' | 'both' | 'Play Store'; |
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.
Do you think this naming is okay? I didn't put much effort into it.
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.
Yes, that should be OK, Thank you for your contribution
Why
Currently, you cannot use
react-native-iap
with expo (at least on android), because you have to configurebuild.gradle
. Building an android app results in the following error:How
I've created config plugin that changes the content of both
build.gradle
files.TODO
AndroidX
support,