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

Check node and yarn #222

Merged
merged 2 commits into from
Apr 4, 2017
Merged

Check node and yarn #222

merged 2 commits into from
Apr 4, 2017

Conversation

gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Apr 2, 2017

Related to #217

@dhh Is this good enough?

screen shot 2017-04-02 at 16 12 56

@matthewd
Copy link
Member

matthewd commented Apr 2, 2017

I don't think we need to be chatty on the happy path.

Is this the first time we're relying on a node executable to be on the path?

@gauravtiwari
Copy link
Member Author

gauravtiwari commented Apr 2, 2017

@matthewd Right, yeah a bit chatty 😄 Yeah, I think so. We have a yarn task in Rails 5.1 - https://github.com/rails/rails/blob/8e9e94391902b854662ba5eb06cc006cc4e85212/railties/lib/rails/generators/rails/app/templates/bin/yarn, but no node check anywhere else.

node_version = `node -v`

unless $?.success?
puts "Node.js not installed"
Copy link
Contributor

@ytbryan ytbryan Apr 2, 2017

Choose a reason for hiding this comment

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

Thanks for this PR @gauravtiwari .

My suggestion. Use Node since all instructions mention Node instead of Node.js for consistency

 puts "Node is not installed"
 puts "Please install node at https://nodejs.org/en/download/ and run bundle exec rails webpacker:install"
 exit!

end

if node_version.tr("v", "").to_f < 6.4
puts "Webpacker require node >= 6.4 and you are using #{node_version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires with a s

Node with a N

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion:

 puts "Webpacker requires Node 6.4.0+ and you are using #{node_version}"
 puts "Please upgrade Node at https://nodejs.org/en/download/ and run bundle exec rails webpacker:install"
exit!

exit!
end

puts "You are using node #{node_version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Node with a N, please

Verify node and yarn before running installer

Take 2

Fix case

Use a new puts

Remove .
@gauravtiwari
Copy link
Member Author

Thanks for reviewing 😄 Pushed some updates, it's less chatty now and uses Node.js to make it more uniform. Apparently, that's how it is everywhere.

screen shot 2017-04-02 at 15 48 26

@rmosolgo
Copy link

rmosolgo commented Apr 3, 2017

👍 thanks @gauravtiwari ! I thought that the installation was successful, but turns out I didn't have Yarn yet:

         run  ./bin/yarn add webpack webpack-merge js-yaml path-complete-extname webpack-manifest-plugin babel-loader coffee-loader coffee-script babel-core babel-preset-env compression-webpack-plugin rails-erb-loader glob extract-text-webpack-plugin node-sass file-loader sass-loader css-loader style-loader postcss-loader autoprefixer postcss-smart-import precss from "."
Yarn executable was not detected in the system.
Download Yarn at https://yarnpkg.com/en/docs/install
Installing dev server for live reloading
         run  ./bin/yarn add --dev webpack-dev-server from "."
Yarn executable was not detected in the system.
Download Yarn at https://yarnpkg.com/en/docs/install
Webpacker successfully installed 🎉 🍰

@dhh dhh merged commit 36f1300 into rails:master Apr 4, 2017
@gauravtiwari gauravtiwari deleted the check-node-and-yarn branch April 5, 2017 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants