[TIMOB-24734](6_1_X) Add compatibility with Hyperloop AAR handling #9087
Conversation
Hyperloop currently still has its own AAR handling which interferes with the new AAR handling in the SDK. This fix will ensure the compatibility by reverting any changes Hyperloop is trying to make to the builder.
|
FR Passed. Checked it with the hyperloop examples app. No crash seen due to duplicate resources with .aar and Hyperloop. Studio Ver: 4.9.0.201705180402 |
android/cli/hooks/aar-transform.js
Outdated
| @@ -9,6 +9,7 @@ | |||
| */ | |||
|
|
|||
| var AarTransformer = require('appc-aar-tools').AarTransformer; | |||
| var appc = require('node-appc'); | |||
There was a problem hiding this comment.
No need to do this. init() is passed a 4th argument which is a node-appc instance.
exports.init = function (logger, config, cli, appc) {You'll have to pass appc into registerHyperloopCompatibilityFixes().
android/cli/hooks/aar-transform.js
Outdated
| @@ -45,6 +46,7 @@ exports.init = function (logger, config, cli) { | |||
| cli.on('build.pre.compile', { | |||
| post: function(builder, callback) { | |||
| scanProjectAndStartTransform(builder, logger, callback); | |||
| registerHyperloopCompatibilityFixes(cli, builder, logger); | |||
There was a problem hiding this comment.
scanProjectAndStartTransform() is async. The call to registerHyperloopCompatibilityFixes() must be done in scanProjectAndStartTransform()'s callback or if there's no conflict, registerHyperloopCompatibilityFixes() could simply be run before calling scanProjectAndStartTransform().
scanProjectAndStartTransform(builder, logger, function (err) {
if (!err) {
registerHyperloopCompatibilityFixes(cli, builder, logger);
}
callback(err);
});
android/cli/hooks/aar-transform.js
Outdated
| */ | ||
| function registerHyperloopCompatibilityFixes(cli, builder, logger) { | ||
| var hyperloopModule = null; | ||
| builder.nativeLibModules.forEach(function (module) { |
There was a problem hiding this comment.
This should be a builder.nativeLibModules.some() call and when you find the hyperloop module, return true; to break the looping.
android/cli/hooks/aar-transform.js
Outdated
| * @param {Object} data Hook data | ||
| * @param {Function} callback Callback function | ||
| */ | ||
| pre: function(data, callback) { |
There was a problem hiding this comment.
Our JavaScript coding style mandates there be a space between constructs and parenthesis. This rule applies to function and while as well as if.
android/cli/hooks/aar-transform.js
Outdated
| pre: function(data, callback) { | ||
| logger.trace('Cleaning AAPT options from changes made by Hyperloop'); | ||
| var aaptOptions = data.args[1]; | ||
| var extraPackagesIndex = aaptOptions.indexOf('--extra-packages') + 1; |
There was a problem hiding this comment.
This is insufficient. indexof() returns -1 if --extra-packages is not found. You have no error handling around this. If it is not found, then extraPackages will always be set to aaptOptions[0].
android/cli/hooks/aar-transform.js
Outdated
| var extraPackagesIndex = aaptOptions.indexOf('--extra-packages') + 1; | ||
| var extraPackages = aaptOptions[extraPackagesIndex]; | ||
| var parameterIndex = aaptOptions.indexOf('-S'); | ||
| while(parameterIndex !== -1) { |
There was a problem hiding this comment.
Missing space after while.
android/cli/hooks/aar-transform.js
Outdated
| var resourcePath = aaptOptions[parameterIndex + 1]; | ||
| if (resourcePath.indexOf(hyperloopBuildPath) !== -1) { | ||
| var manifestPathAndFilename = path.join(resourcePath, '../AndroidManifest.xml'); | ||
| if (fs.existsSync(manifestPathAndFilename)) { |
There was a problem hiding this comment.
FYI, fs.existsSync() has been deprecated. It's fine for now because it's everywhere else and we'd have to polyfill it anyways. The preferred way is calling fs.statSync() with a try/catch.
android/cli/hooks/aar-transform.js
Outdated
| var manifestPathAndFilename = path.join(resourcePath, '../AndroidManifest.xml'); | ||
| if (fs.existsSync(manifestPathAndFilename)) { | ||
| var manifestContent = fs.readFileSync(manifestPathAndFilename).toString(); | ||
| var packageNameMatch = manifestContent.match(/package="(.*)"/); |
There was a problem hiding this comment.
Since this is in a loop, it is preferred that the regex be compiled ahead of time.
android/cli/hooks/aar-transform.js
Outdated
| var packageNameMatch = manifestContent.match(/package="(.*)"/); | ||
| if (packageNameMatch !== null) { | ||
| var packageName = packageNameMatch[1]; | ||
| extraPackages = extraPackages.replace(':' + packageName, ''); |
There was a problem hiding this comment.
It's safer to split on the separator and re-join:
extraPackages = extraPackages.split(':').filter(n => n === packageName).join(':');
janvennemann
force-pushed
the
TIMOB-24734-6_1_X
branch
from
06c4d96
to
fc6982d
Compare
android/cli/hooks/aar-transform.js
Outdated
| logger.trace('Cleaning AAPT options from changes made by Hyperloop'); | ||
| var aaptOptions = data.args[1]; | ||
| var extraPackagesIndex = aaptOptions.indexOf('--extra-packages') + 1; | ||
| if (extraPackagesIndex === -1) { |
There was a problem hiding this comment.
Should this be:
if (extraPackagesIndex === 0) {?
|
FR for new changes passed.
Studio Ver: 4.9.0.201705180402 |
JIRA: https://jira.appcelerator.org/browse/TIMOB-24734
6_1_X backport of #9086