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
[TIMOB-25028] Fix issue where node-ios-device is not hoisted #9260
Conversation
build/packager.js
Outdated
cb(); | ||
}); | ||
if (this.targetOS === 'osx') { | ||
console.log(path.join(this.zipSDKDir, 'node_modules', 'node-ios-device')); |
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.
Is the console.log()
debug? It should probably be removed.
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.
Removed
build/packager.js
Outdated
} | ||
cb(); | ||
}); | ||
} else if (fs.existsSync(normalPath)) { |
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.
You have a lot of duplicate code. It can be simplified:
let dir = path.join(this.zipSDKDir, 'node_modules', 'node-ios-device');
if (!fs.existsSync(dir)) {
dir = path.join(this.zipSDKDir, 'node_modules', 'ioslib', 'node_modules', 'node-ios-device');
}
if (!fs.existsSync(dir)) {
return cb(new Error('Unable to find node-ios-device module'));
}
exec('node bin/download-all.js', { cwd: dir, stdio: 'inherit' }, cb);
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.
Yeah my bad I was thinking that it could be better as I wrote it, updated. I kept the exec usage similar to the others throughout the file,
build/packager.js
Outdated
return cb(new Error('Unable to find node-ios-device module')); | ||
} | ||
|
||
exec('node bin/download-all.js', {cwd: dir}, function (err, stdout, stderr) { |
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.
I would just inherit stdio, then you don't need to manually print the stdout/stderr. You can just pass in cb
like this:
exec('node bin/download-all.js', { cwd: dir, stdio: 'inherit' }, cb);
Also note that we put spaces inside braces/brackets around the props/elements.
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.
Couldn't have written it better myself. APPROVED!
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.
FT also works great!
JIRA: https://jira.appcelerator.org/browse/TIMOB-25028
node-ios-device can be in
node_modules
ornode_modules/ioslib/node_modules
we should consider that when executing the script.Verification