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

[CLI] Remove dependency on Ruby #440

Merged
merged 5 commits into from
May 14, 2015
Merged

[CLI] Remove dependency on Ruby #440

merged 5 commits into from
May 14, 2015

Conversation

JoeStanton
Copy link
Contributor

Resolves #275, #293. Discussed in #405.

Rewritten the init script in Node to remove the dependency on Ruby. I've also moved it into the react-native-cli directory as I thought it was cleaner.

@vjeux
Copy link
Contributor

vjeux commented Mar 29, 2015

Sweet!

init.sh should be outside of the cli folder. Since the cli is global, it's super hard to update. What it does is just forward all the commands to react-native which can be updated easily.

@JoeStanton JoeStanton force-pushed the init-to-nodejs branch 2 times, most recently from 7fb25db to 1a2357b Compare March 29, 2015 15:13
@JoeStanton
Copy link
Contributor Author

Ah that makes sense, global packages are annoying to update. Moved it back out.

@vjeux
Copy link
Contributor

vjeux commented Mar 29, 2015

Looks good, i'll test it and merge it later today or tomorrow, thanks!

@JoeStanton JoeStanton mentioned this pull request Mar 30, 2015
Merged
@frantic frantic mentioned this pull request Mar 30, 2015
Closed
frantic
frantic reviewed Mar 30, 2015
View reviewed changes
init.sh
@@ -1,44 +1,50 @@
#!/usr/bin/env ruby
#!/usr/bin/env node
Copy link
Contributor

Choose a reason for hiding this comment

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

This file no longer has to be invoked via shell. Since it's all JS, you can just require and run it from cli.js

@frantic
Copy link
Contributor

frantic commented Mar 30, 2015

Awesome job @JoeStanton! I have a few suggestions - see inline comments

iamdustan
iamdustan reviewed Mar 30, 2015
View reviewed changes
init.sh
}
end
if (process.argv.length === 0) {
console.log('Usage: ' + path.basename(__filename) + ' <ProjectNameInCamelCase>');
Copy link

Choose a reason for hiding this comment

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

Not 100% on this, but from when I started this over the weekend as well, I think that ruby’s __FILE__ returns just the filename (init.sh), while node’s __filename includes the full path resolution (/Users/user/path/to/react-native/init.sh).

@iamdustan iamdustan mentioned this pull request Mar 30, 2015
Closed
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 7, 2015
@nicklockwood
Copy link
Contributor

@frantic what happened to this? Can we resurrect it?

@JoeStanton
Copy link
Contributor Author

Hey! My fault - Life got in the way for a few weeks. I can finish this up in the next couple of days if there's still the appetite for it?

@nicklockwood
Copy link
Contributor

@JoeStanton yes please!

@JoeStanton
1-1 port of init script to Nodejs
132f8fa 
This will remove the dependency on Ruby.
@JoeStanton JoeStanton force-pushed the init-to-nodejs branch 2 times, most recently from 2f43e1d to 71abc5f Compare May 14, 2015 14:24
@JoeStanton JoeStanton force-pushed the init-to-nodejs branch 3 times, most recently from b15f5ed to 526890d Compare May 14, 2015 14:56
@JoeStanton JoeStanton force-pushed the init-to-nodejs branch 2 times, most recently from 8571ce3 to 52e7b80 Compare May 14, 2015 15:07
@JoeStanton
Copy link
Contributor Author

Hey Guys - Finally had a little bit of time, can you take a look at this? AFAIK I can't put init.js inside the local-cli folder as @vjeux said previously. Does this still apply?

Secondly, the init.js could be cleaned up nicely with some ES6 features, but I've targeted ES5 for the moment unless theres a way to run it through the packager (probably overkill).

@vjeux
Copy link
Contributor

vjeux commented May 14, 2015

@JoeStanton you can't put it in react-native-cli, but you CAN put it in local-cli (that's the right place where to put it)

@JoeStanton
Copy link
Contributor Author

Excellent - fixed that up.

vjeux
vjeux reviewed May 14, 2015
View reviewed changes
local-cli/init.js
}

function walk(current) {
if(fs.lstatSync(current).isDirectory()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after if

@vjeux vjeux assigned vjeux and unassigned frantic May 14, 2015
@vjeux
Copy link
Contributor

vjeux commented May 14, 2015

Thanks, a few minor details and we're good to go :)

@vjeux
Copy link
Contributor

vjeux commented May 14, 2015

Oh you are right, nevermind me

vjeux added a commit that referenced this pull request May 14, 2015
@vjeux
Merge pull request #440 from JoeStanton/init-to-nodejs
b2d1d6f 
[CLI] Remove dependency on Ruby
@vjeux vjeux merged commit b2d1d6f into facebook:master May 14, 2015
@JoeStanton JoeStanton deleted the init-to-nodejs branch May 14, 2015 20:40
@JoeStanton JoeStanton mentioned this pull request May 14, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@vjeux vjeux

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JoeStanton @vjeux @frantic @nicklockwood @iamdustan @facebook-github-bot @reactjs-bot