-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Loosen up yarn and support npm #475
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,7 +75,7 @@ in which case you may not even need the asset pipeline. This is mostly relevant | |
* Ruby 2.2+ | ||
* Rails 4.2+ | ||
* Node.js 6.4.0+ | ||
* Yarn 0.20.1+ | ||
* Yarn 0.20.1+ or NPM 4.2.0+ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why 4.2 and not 2, or even 1? All the npm commands here work in every version of npm. Why artificially restrict it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are they compatible with all versions of node? In webpacker node > 6.4 is required. I noticed later npm versions dropped support for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. npm 4.6+ doesn't work in node < v1, and npm 5+ doesn't work in node < 4. But, npm 2 works with node 6, and I believe with node 8 too. In other words, npm might have a node requirement, but node tends to always work with older npms. I absolutely would want someone to verify that a version of npm the docs claimed to work, works :-) and it's fine if it says 4.2 because nobody's tested older; but it'd be ideal to find the absolute lowest version of npm that works with node 6.4+ (which includes the latest version of npm 2, at the least, of that I'm certain). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should be supporting node < 4.2, as it is most of what's under const { settings, output } = require('./configuration.js') So as far as I'm concerned webpacker already only supports node 4.2+ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like you want node 4.2+, but that's still not a reason to restrict npm to 4.2. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nit, but "npm" is always lowercase: https://twitter.com/seldo/status/850042251635867648 |
||
|
||
|
||
## Features | ||
|
@@ -289,6 +289,7 @@ If you want to add a new loader, for example, to process `json` files via webpac | |
|
||
``` | ||
yarn add json-loader | ||
npm install --save json-loader | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. npm >=5 will save by default, so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's only true if |
||
``` | ||
|
||
And create a `json.js` file inside `loaders` directory: | ||
|
@@ -706,6 +707,7 @@ To add any new JS module you can use `yarn`: | |
|
||
```bash | ||
yarn add bootstrap material-ui | ||
npm install --save material-ui | ||
``` | ||
|
||
|
||
|
@@ -715,6 +717,7 @@ You can use yarn to add bootstrap or any other modules available on npm: | |
|
||
```bash | ||
yarn add bootstrap | ||
npm install --save bootstrap | ||
``` | ||
|
||
Import Bootstrap and theme(optional) CSS in your app/javascript/packs/app.js file: | ||
|
@@ -741,6 +744,7 @@ for using typescript with React: | |
|
||
```bash | ||
yarn add ts-loader typescript @types/react @types/react-dom | ||
npm install --save ts-loader typescript @types/react @types/react-dom | ||
|
||
# You don't need this with typescript | ||
yarn remove prop-types | ||
|
@@ -794,6 +798,7 @@ you would need to follow these steps to add HTML templates support: | |
|
||
```bash | ||
yarn add html-loader | ||
npm install --save html-loader | ||
``` | ||
|
||
2. Add html-loader to `config/webpacker/loaders/html.js` | ||
|
@@ -946,6 +951,7 @@ You can also use [babel-plugin-module-resolver](https://github.com/tleunen/babel | |
|
||
```bash | ||
yarn add babel-plugin-module-resolver | ||
npm install --save babel-plugin-module-resolver | ||
``` | ||
|
||
Specify the plugin in your `.babelrc` with the custom root or alias. Here's an example: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
require "webpacker/node_bundler" | ||
|
||
node_bundler = Webpacker::NodeBundler.command | ||
node_bundler_dev = Webpacker::NodeBundler.command_dev | ||
|
||
# Install webpacker | ||
copy_file "#{__dir__}/config/webpacker.yml", "config/webpacker.yml" | ||
|
||
|
@@ -28,7 +33,7 @@ | |
end | ||
|
||
puts "Installing all JavaScript dependencies" | ||
run "yarn add webpack webpack-merge js-yaml path-complete-extname " \ | ||
run "#{node_bundler} webpack webpack-merge js-yaml path-complete-extname " \ | ||
"webpack-manifest-plugin [email protected] coffee-loader coffee-script " \ | ||
"babel-core babel-preset-env babel-polyfill compression-webpack-plugin rails-erb-loader glob " \ | ||
"extract-text-webpack-plugin node-sass file-loader sass-loader css-loader style-loader " \ | ||
|
@@ -37,6 +42,6 @@ | |
"babel-plugin-transform-object-rest-spread" | ||
|
||
puts "Installing dev server for live reloading" | ||
run "yarn add --dev webpack-dev-server" | ||
run "#{node_bundler_dev} webpack-dev-server" | ||
|
||
puts "Webpacker successfully installed 🎉 🍰" |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# Finds available global or local node package manager - yarn or npm | ||
|
||
require "webpacker/configuration" | ||
|
||
module Webpacker::NodeBundler | ||
extend self | ||
|
||
def name | ||
@package ||= packages.find(-> { raise Errno::ENOENT }) { |package| available?(package) } | ||
|
||
rescue Errno::ENOENT | ||
$stderr.puts "Node package managers npm and yarn not found! Scanned following paths for executables - #{packages}" | ||
$stderr.puts "Make sure either npm or yarn is installed globally, or locally within your app node_modules folder." | ||
exit! | ||
end | ||
|
||
def command | ||
commands[name] | ||
end | ||
|
||
def command_dev | ||
commands[:dev][name] | ||
end | ||
|
||
private | ||
|
||
def available?(package) | ||
null_device = Gem.win_platform? ? "/nul 2>&1" : "/dev/null 2>&1" | ||
system("#{package} --version > #{null_device}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many developers will have both yarn and npm installed. Seems like we just pick yarn if first. Should that be documented? Maybe this should just be in the config file or via an ENV, b/c those devs working on multiple projects will surely have yarn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "surely" might be a bit strong - I work on hundreds of projects and don't have yarn installed for any of them. I also think the presence of yarn - which perhaps one package I work on might require - shouldn't force it to use yarn. Explicit configuration when it's ambiguous might be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @justin808 The package manager is checked in order - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree npm will generally always be available; I'm suggesting that the mere presence of yarn doesn't indicate a preference for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A |
||
end | ||
|
||
def commands | ||
commands = { dev: {} } | ||
|
||
yarn.each do |package| | ||
commands["#{package}"] = "#{package} add" | ||
commands[:dev]["#{package}"] = "#{package} add --dev" | ||
end | ||
|
||
npm.each do |package| | ||
commands["#{package}"] = "#{package} install --save" | ||
commands[:dev]["#{package}"] = "#{package} install --save-dev" | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gauravtiwari Will there be too much maintenance to support (every version of) npm and yarn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No don't think so - that was the idea actually to support everything available so, we don't say "don't have yarn, buzz off" |
||
|
||
commands | ||
end | ||
|
||
def packages | ||
[*yarn, *npm] | ||
end | ||
|
||
def yarn | ||
%W( | ||
yarn | ||
#{Webpacker::Configuration.node_modules_bin_path}/yarn | ||
yarnpkg | ||
#{Webpacker::Configuration.node_modules_bin_path}/yarnpkg | ||
).freeze | ||
end | ||
|
||
def npm | ||
%W( | ||
npm | ||
#{Webpacker::Configuration.node_modules_bin_path}/npm | ||
).freeze | ||
end | ||
end |
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.
y
is extra, I think 👀