enhancement: Control project properties in one place #1211
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1211 +/- ##
=========================================
Coverage ? 72.14%
=========================================
Files ? 22
Lines ? 1709
Branches ? 0
=========================================
Hits ? 1233
Misses ? 476
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
It's great to see that someone is tackling this issue. It is something that has bugged me for a long time 👍.
I left a few comments, most of which concern only minor details. One big thing that confuses me is the variable interpolation applied to various files in create.js. It seems that only one file is actually making use of the interpolation. Is this a leftover from a previous version or am I missing something here?
It would be great if we had as little templating as possible IMHO. It just adds another layer of complexity. And if project.properties really was the only file that used templating, I'd suggest a simpler approach for it than the TemplateFile class.
| privateHelpers.getProjectTarget = { doGetProjectTarget() } | ||
| privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools('19.1.0') } | ||
| privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools("29.0.2") } |
There was a problem hiding this comment.
Is that version not something we can/should move to a config? I'm seriously asking, no idea.
There was a problem hiding this comment.
Yah, in fact we already have a variable for it, so good catch
| DEFAULT_SDK_VERSION: targetAPI || constants.DEFAULT_SDK_VERSION, | ||
| DEFAULTS_FILE_PATH: './defaults.json' | ||
| }); | ||
| TemplateFile.render(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'), { |
There was a problem hiding this comment.
The only file that seems to actually make use of interpolating template variables is project.properties. Did you refactor something and this is leftovers or did I not get something?
There was a problem hiding this comment.
Yah -- I must have forgot to revert. Original (unreleased) version of this PR actually used them, but then I refactored so that the gradle environment can just load in a JSON file itself (which allowed me to also utilize these constants for our test environments)
There was a problem hiding this comment.
OK. I think the JSON file is definitely the better solution 👍
|
Thanks @raphinesse for the in-depth review. Obviously I still have some work to do so I set this to a draft for now. I'll try to clean up the PR and address all the concerns you've brought up this weekend coming. |
Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
breautek
force-pushed
the
enhancements/constant-power
branch
from
39c08af
to
c7e02e0
Compare
Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
breautek
force-pushed
the
enhancements/constant-power
branch
from
c7e02e0
to
26897e2
Compare
|
Closing this in favour of #1212 |
Platforms affected
Android
Motivation and Context
Currently when we want to bump certain variables such as our gradle version, we have several files we need to keep track of and update. This is an enhancement that allows us to define a single variable such as the gradle version, or our target/min SDK versions in a single place.
Description
framework now contains a
defaults.jsonfile which contains several constants such the gradle version, among other constants which is used throughout both nodejs and gradle to set our defaults values.Our androidx native unit test also makes use of this file to set its min sdk/target sdk/build tools, etc.
If there is anything else that should be included in the defaults constants I'd be happy to add them.
Testing
npm test and manual testing
Checklist
(platform)if this change only applies to one platform (e.g.(android))