Skip to content
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

fix(ios): append TitaniumKit.xcframework if module is created with sdk < 9.2.0 #12153

Merged
merged 3 commits into from Oct 5, 2020

Conversation

vijaysingh-axway
Copy link
Contributor

@build
Copy link
Contributor

build commented Oct 2, 2020

Fails
🚫 Tests have failed, see below for more information.
Warnings
⚠️

iphone/cli/commands/_buildModule.js#L157 - iphone/cli/commands/_buildModule.js line 157 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)

⚠️

iphone/cli/commands/_buildModule.js#L157 - iphone/cli/commands/_buildModule.js line 157 – Expected catch() or return (promise/catch-or-return)

Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 2 tests have failed There are 2 tests failing and 742 skipped out of 8343 total tests.

Tests:

ClassnameNameTimeError
ios.ipad.Titanium.UI.iOS.CollisionBehavior.exampleworks (14.0)15
Error: timeout of 15000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/0035AEF4-D58B-4282-9C16-4D3301CDDBFC/data/Containers/Bundle/Application/5BCDE894-2448-4D31-AF9A-B15528F1C0B5/mocha.app/ti-mocha.js:4326:27
ios.macos.Titanium.UI.iOS.CollisionBehavior.exampleworks (10.15.5)15.147
Error: timeout of 15000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-12153/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27

Generated by 🚫 dangerJS against 78e92f5

@lokeshchdhry
Copy link
Contributor

Works for me. Want some more eyes on this from other QE's.

@@ -237,6 +252,60 @@ iOSModuleBuilder.prototype.processLicense = function processLicense() {
};

iOSModuleBuilder.prototype.processTiXcconfig = function processTiXcconfig(next) {
const srcFile = path.join(this.projectDir, this.moduleName + '.xcodeproj', 'project.pbxproj');
const xcodeProject = xcode.project(path.join(this.projectDir, this.moduleName + '.xcodeproj', 'project.pbxproj'));
const contents = fs.readFileSync(srcFile).toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, when reading a file, you can avoid converting from Buffer to String (calling toString()), by passing in an encoding argument to the readFile call: fs.readFileSync(srcFile, 'utf8');

this.logger.warn(`Module created with sdk < 9.2.0, need to add TitaniumKit.xcframework. The ${this.moduleName}.xcodeproj has been updated.`);

xcodeProject.hash = xcodeParser.parse(contents);
const xobjs = xcodeProject.hash.project.objects,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a style thing in JS - in the old days you should use var and declare all variables at the top of the scope, because JS hoisted them to the top of the scope anyways, so we wanted to match in source to avoid inconsistency. But with let/const, they're more like variables in any other language, they're more or less treated as declared/initialized where you define them, so you don't need to stick them at the top of the block/scope. Ideally we'd break up this big block of declared variables and move them to be declared where they're needed to make it easier to follow the logic and which grouping of variables applies to what lines of code used to edit/process them.

This is a long way of saying, most of these variables aren't needed until lines 294/299 below.

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM.

For older modules, do they have a TitaniumKit.framework reference that needs to be removed/replaced by the xcframework reference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants