[TIMOB-24829] Improve android build performance #205
Conversation
janvennemann
force-pushed
the
TIMOB-24829
branch
from
0d8d2f0
to
4bc25c3
Compare
janvennemann
force-pushed
the
TIMOB-24829
branch
from
4bc25c3
to
5341c33
Compare
| } | ||
| // Special case for android-support-v4.jar and android-support-v13.jar | ||
| if (basenames.hasOwnProperty('android-support-v4.jar') && basenames.hasOwnProperty('android-support-v13.jar')) { | ||
| var specialCase = []; |
There was a problem hiding this comment.
uniqueJars = uniqueJars.filter(function (jarFile) {
return path.basename(jarFile) !== 'android-support-v4.jar';
});
| canDoIncrementalRun() { | ||
| let buildManifest = this._builder.buildManifest; | ||
|
|
||
| if (buildManifest.deployType !== this._builder.deployType) { |
There was a problem hiding this comment.
Do we really care about deploy type? Only in that it changes the default setting for magnification, right? Does that not get reflected int he check below?
There was a problem hiding this comment.
Only the deploy type is stored in the build manifest so i use that to indirectly check wether the minify JS settings changed. The other check is for the --skip-minify-js flag which can be changed regardless of deploy type.
| minifyJsAndWrite(sourcePath, destinationPath) { | ||
| return new Promise( | ||
| (resolve, reject) => { | ||
| babel.transformFile(sourcePath, { |
There was a problem hiding this comment.
We should really have a built in "magnification" task/library whatever to call here. We have found that sometimes plugins in babili result in busted JS that some engines can't handle, and have had to tweak the minification before. I would hope we could just ask the CLI build process to minify these files as some build step and not have to have babili in hyperloop again and call it with possibly differing options/plugin versions/etc.
There was a problem hiding this comment.
Yes, totally agree on that! I didn't feel comfortable using babili directly but it was the only way to get minification as i explicitly avoid the default build pipeline as that was the performance bottle neck. A way to call the minification from node-titanium-sdk without the overhead of running the JS analyzation would be nice so we could drop the direct dependency to babili in Hyperloop again.
| /** | ||
| * Constructs a new task for metabase generation | ||
| * | ||
| * @param {Obejct} taskInfo |
| * @return {Promise} | ||
| */ | ||
| runTaskAction() { | ||
| let inputFiles = Array.from(this.inputFiles); |
| return this.doFullTaskRun(); | ||
| } | ||
|
|
||
| let referencedClasses = []; |
There was a problem hiding this comment.
refactor out a method to re-use same code as in full run to gather the full set of referenced classes?
| }); | ||
|
|
||
| let expandedClassList = metabase.generate.expandDependencies(this.metabase, referencedClasses); | ||
| let classesToGenerate = expandedClassList.filter(className => !this._generatedClasses.has(className)); |
There was a problem hiding this comment.
This plus the following code is a good use case for https://lodash.com/docs/#partition if we're using lodash... Looks like you're basically looping through this._generatedClasses. If it's in expandedClassesList, we should generate it, if not, we should remove it (then do a loop on the set to remove and actually delete them, preferably async).
|
|
||
| changedFiles.forEach((state, pathAndFilename) => { | ||
| if (state === 'created' || state === 'changed') { | ||
| let hyperloopUsed = this.scanFileForHyperloopRequires(pathAndFilename); |
| this._references.forEach((fileInfo, pathAndFilename) => { | ||
| referencesObj[pathAndFilename] = fileInfo; | ||
| }); | ||
| let referencesJson = JSON.stringify(referencesObj); |
| */ | ||
| scanFileForHyperloopRequires(pathAndFilename) { | ||
| var result = this.extractAndReplaceHyperloopRequires(pathAndFilename); | ||
| if (result !== null && result.usedClasses.length > 0) { |
There was a problem hiding this comment.
Do we care if result is null or undefined? Why not justif (result && result.usedClasses.length > 0) {?
| return null; | ||
| } | ||
|
|
||
| var contents = fs.readFileSync(file, 'UTF-8'), |
There was a problem hiding this comment.
can these all be const?
| requireRegex = /require\s*\(\s*[\\"']+([\w_/-\\.\\*]+)[\\"']+\s*\)/ig; | ||
| this._logger.trace('Searching for hyperloop requires in: ' + file); | ||
| (contents.match(requireRegex) || []).forEach(m => { | ||
| var re = /require\s*\(\s*[\\"']+([\w_/-\\.\\*]+)[\\"']+\s*\)/i.exec(m), |
There was a problem hiding this comment.
isn't this the same regexp from above? I think this should be removed, and className = re[1], should be const classname = m[1],
| (contents.match(requireRegex) || []).forEach(m => { | ||
| var re = /require\s*\(\s*[\\"']+([\w_/-\\.\\*]+)[\\"']+\s*\)/i.exec(m), | ||
| className = re[1], | ||
| lastIndex, |
There was a problem hiding this comment.
Move to using let/const in the proper scopes below, please...
| * @param {String} replaceStr String used to replace all occurrences of needle | ||
| * @return {String} New string which has all occurrences of needle replaced | ||
| */ | ||
| replaceAll(haystack, needle, replaceStr) { |
There was a problem hiding this comment.
I think I wrote this horrible code :( Maybe improve it a little bit...
replaceAll(haystack, needle, replaceStr) {
const length = needle.length;
let newBuffer = haystack,
index = -1;
while ((index = newBuffer.indexOf(needle)) >= 0) {
const before = newBuffer.substring(0, index);
const after = newBuffer.substring(index + length);
newBuffer = before + replaceStr + after;
}
return newBuffer;
}There was a problem hiding this comment.
Haha, i didn't want to touch any of the existing code and rather focused on moving into a task for now. I'll implement this improvement
| }, | ||
| "nyc": { | ||
| "reporter": [ | ||
| "lcov", |
There was a problem hiding this comment.
So if we produce a 'cobertura' coverage report, Jenkins can consume that...
There was a problem hiding this comment.
I'll add that!
|
Yay for tests! Do these actually run as part of the build? I'm guessing not. Can we please hook them up in the Jenkinsfile? Ideally we'd model this like many of the other npm packages, and have a Gruntfile that can do linting/testing of the JS code. If you'd like I can get us started in that direction and then we can hook the nyc/test stuff into it? |
Proper usage of const and let, refactoring of the scan references task to optimize some old code pieces and async removal of unused wrappers in the generate surces task.
JIRA: https://jira.appcelerator.org/browse/TIMOB-24829
This is the reworked version of #189, using the appc-tasks module and complete with unit tests backing the refactored code.
Change overview:
Resourcesdirectory which caused some confusion in classic apps. The hook will now generate all files under thebuilddirectory.To properly work in master, this has to be tested together with #206