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.

Sign up for GitHub

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

Jump to bottom

CB-8197 Switch to nodejs for ios platform scripts #146

Closed
wants to merge 1 commit into from
Closed

CB-8197 Switch to nodejs for ios platform scripts #146

wants to merge 1 commit into from

Conversation

daserge
Copy link
Contributor

@daserge daserge commented Jun 11, 2015

Convert copy-www-build-step.sh to nodejs
Adds glob module and bundledDependencies to package.json

Jira issue

CB-8197 Switch to nodejs for ios platform scripts
21d4e74 
Convert copy-www-build-step.sh to nodejs
Adds glob module and bundledDependencies to package.json
nikhilkh
nikhilkh reviewed Jun 16, 2015
View reviewed changes
bin/templates/project/__CLI__.xcodeproj/project.pbxproj
@@ -374,7 +374,7 @@
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "cordova/lib/copy-www-build-step.sh";
shellScript = "node cordova/lib/copy-www-build-step.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

This change assumes node to be in the $PATH for this to work. I'm not familiar with OS X and I'm assuming this assumption does not cause a problem.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about this earlier when the PR came in and I think its a good assumption since it's a Cordova project and the user is using the CLI. The scenario where a user would take the project from platforms/ios and run it stand-alone is miniscule.

Not sure if this is supported fully but Xcode.app does include node.js under /Applications/Xcode.app/Contents/Developer/usr/share/xcs/Node/bin/node

You would dynamically get this path by using xcode-select --print-path then appending /usr/share/xcs/Node/bin/node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @shazron, @nikhilkh,
Thanks for your review.
Regarding node' path - I propose to leave it as is taking into account that stand-alone case is uncommon.
We can address it later in case of issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 The cli dependent on node, so it makes sense to require it in the path.
Does this affect users who are not using the cli? Probably still not a big deal either way.

@nikhilkh
Copy link
Contributor

LGTM. Adding glob does increase the size of ios platform considerably. I'm assuming there is no other cheaper way to do this.

@asfgit asfgit closed this in 2009cd9 Jun 24, 2015
asfgit pushed a commit that referenced this pull request Jul 24, 2015
@sgrebnov
CB-8197 Switch to nodejs for ios platform scripts
cef9b1f 
Convert copy-www-build-step.sh to nodejs
Adds glob module and bundledDependencies to package.json. This closes #146
@janpio janpio mentioned this pull request Mar 1, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daserge @nikhilkh @shazron @purplecabbage