Loosen up yarn and support npm #475

gauravtiwari · 2017-06-05T22:28:24Z

Fixes - #452 and #405

Adds

Detects node package managers in order - yarn, npm and finally fails with an error message if none found instead of just depending on Yarn

Removes

Extra yarn:install task - I feel we leave this to user like bundle install - Heroku already takes cares of this if we are using webpacker or node buildpack.
Loosen up yarn usage inside binstubs

Feel free to point out any platform specific (particularly windows) thing that needs fixing here 👍

Use Rails.root Fix node_modules_path Fix text Prefix ruby path to executable

gauravtiwari · 2017-06-05T22:30:06Z

// cc @gdborton @ljharb @Gargron @himdel

ljharb · 2017-06-05T22:32:48Z

lib/install/elm.rb

-run "yarn run elm package install -- --yes"
+run "#{node_bundler} elm elm-webpack-loader"
+run "#{node_bundler} --dev elm-hot-loader"
+run "#{node_bundler} elm package install -- --yes"


i think this might need to be ${Webpacker::Configuration.node_modules_bin_path}/elm?

ljharb · 2017-06-05T22:33:47Z

lib/webpacker/node_bundler.rb

+
+ rescue Errno::ENOENT
+ $stderr.puts "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 node_modules folder."


i believe both npm and yarn are always supposed to be lowercased

ljharb · 2017-06-05T22:34:19Z

lib/webpacker/node_bundler.rb

+ {
+ yarn: "yarn add",
+ yarnpkg: "yarnpkg add",
+ npm: "npm install"


you may want npm install --save here, to mimic what yarn add does

if you do, remember to write it in readme :)

ljharb · 2017-06-05T22:34:31Z

lib/install/bin/webpack-dev-server.tt

@@ -36,7 +36,7 @@ newenv = {
 "ASSET_HOST" => DEV_SERVER_HOST.shellescape
 }.freeze

-cmdline = ["yarn", "run", "webpack-dev-server", "--", "--progress", "--color", "--config", WEBPACK_CONFIG] + ARGV
+cmdline = ["#{NODE_MODULES_PATH}/.bin/webpack-dev-server", "--progress", "--color", "--config", WEBPACK_CONFIG] + ARGV


#{Webpacker::Configuration.node_modules_bin_path}/?

ljharb · 2017-06-05T22:34:35Z

lib/install/bin/webpack.tt

@@ -20,7 +20,7 @@ unless File.exist?(WEBPACK_CONFIG)
 end

 newenv = { "NODE_PATH" => NODE_MODULES_PATH.shellescape }
-cmdline = ["yarn", "run", "webpack", "--", "--config", WEBPACK_CONFIG] + ARGV
+cmdline = ["#{NODE_MODULES_PATH}/.bin/webpack", "--config", WEBPACK_CONFIG] + ARGV


#{Webpacker::Configuration.node_modules_bin_path}/?

guilleiguaran · 2017-06-06T06:12:06Z

lib/tasks/webpacker/check_yarn.rake

-namespace :webpacker do
- desc "Verifies if yarn is installed"
- task :check_yarn do
- required_yarn_version = "0.20.1"


We would want to keep the version checking for Yarn

Not sure if we are not using it exclusively. I noticed these checks are taking a second or two and slowing down the installer overall. We already documented the version requirements in README anyway and perhaps re-iterate that again inside installation section too?

If slowness is the reason to remove it, do you think adding option to disable checking, will be better than removing it?

guilleiguaran · 2017-06-06T06:14:38Z

The main reason the choose Yarn over npm when we started the integration between Rails and Yarn was the lockfiles so I think we would prefer to support only npm >= 5

ljharb · 2017-06-06T06:45:57Z

@guilleiguaran you don't have to do anything special to support npm < 5, so that's not really a useful distinction to make - all the commands are the same.

Separately, though, "use a lockfile" is something that really should be up to the user, even if the default is to have one.

gauravtiwari · 2017-06-06T06:58:09Z

@ljharb @ytbryan Thanks for reviewing ❤️ Will fix those

@guilleiguaran Internally we don't care for either yarn or npm as long as node dependencies are satisfied. Obviously there are some benefits of one over the other as you pointed out but lets leave that to user. Perhaps, best to document this in README.

Secondly, since npm comes bundled by default with node it's likely most people will have that installed instead of yarn so the on-boarding process will be much smoother instead of installing yarn straightaway just to try out webpacker.

guilleiguaran · 2017-06-06T07:24:26Z

Don't support npm < 5 in webpacker/Rails was something we did on purpose. Even when we had the option to support both from the start at the time we were satisfied with Yarn because we thought lockfiles were something essential and the lacking of them were the reason we didn't integrate npm with Rails before (See [1] and [2] for reference).

[1] https://twitter.com/dhh/status/808349264216625156
[2] rails/rails#26836

ljharb · 2017-06-06T08:00:11Z

Yes, and those who use npm 5 will get that with this PR.

How exactly do you propose forbidding npm < 5, short of a messy version sniff? Why would that benefit users, if npm < 5 works identically except for how it interacts with package.json and a lockfile?

ljharb · 2017-06-06T08:30:22Z

README.md

@@ -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+


why 4.2 and not 2, or even 1? All the npm commands here work in every version of npm. Why artificially restrict it?

Are they compatible with all versions of node? In webpacker node > 6.4 is required. I noticed later npm versions dropped support for node ~> 0.12

with yarn for example a couple of issues were created relating to a particular version of yarn not working well with installed node version - so, I just wanted to make this actually works ;)

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).

I don't think we should be supporting node < 4.2, as it is most of what's under config/webpacker won't run on node < 4.2, e.g.:

const { settings, output } = require('./configuration.js')

So as far as I'm concerned webpacker already only supports node 4.2+

Sounds like you want node 4.2+, but that's still not a reason to restrict npm to 4.2.

Minor nit, but "npm" is always lowercase: https://twitter.com/seldo/status/850042251635867648

himdel · 2017-06-06T12:02:37Z

CHANGELOG.md

+
+## Removed
+
+* Extra `yarn:install` task - [#405](https://github.com/rails/webpacker/issues/405)


Probably a typo - webpacker:yarn_install is the task that got removed.

Gargron · 2017-06-06T12:08:08Z

@guilleiguaran

Don't support npm < 5 in webpacker/Rails was something we did on purpose. Even when we had the option to support both from the start at the time we were satisfied with Yarn because we thought lockfiles were something essential and the lacking of them were the reason we didn't integrate npm with Rails before (See [1] and [2] for reference).

I feel like this PR is on the right track - Rails shouldn't care if the developer is using yarn, as long as all the needed dependencies have been pulled down.

himdel · 2017-06-06T12:09:12Z

lib/install/elm.rb

-run "yarn add --dev elm-hot-loader"
-run "yarn run elm package install -- --yes"
+run "#{node_bundler} elm elm-webpack-loader"
+run "#{node_bundler} --dev elm-hot-loader"


Probably wrong. yarn add --dev works but node install --save --dev should be spelled --save-dev.

Curiously enough -D looks like it should work with both (yarn -D is yarn --dev while npm -D is npm install --save-dev, and calling npm install --save --save-dev does the right thing (dev wins)).

npm -D won't work, as you'll need (at least) to specify the npm command, install.

Thanks, edited :) (The idea was, not to have to split node_bundler into 2)

himdel · 2017-06-06T12:10:26Z

lib/install/template.rb

@@ -37,6 +41,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"


Same here, --dev does not work with npm.

javan · 2017-06-06T14:38:14Z

Big +1 for supporting npm in addition to yarn, but I think that attempting to do so automatically is going too far. As already seen from the comments, there are numerous versions and varying command arguments to consider.

What do you think of making the package manager configurable in webpacker.yml? It could default to using yarn, and people can adjust as needed. Something like:

# webpacker.yml
default: &default
  package_manager: yarn
  package_add_args: add
  package_install_args: install

dhh · 2017-06-06T16:30:33Z

Let's take a step back here. The reason we went with Yarn in the first place was pretty much exclusively that it provided a deterministic install via the lock file. Now that npm 5 provides the same functionality, I don't think we actually need Yarn any more. And I don't feel like Rails needs to bend over backwards to provide both. Maybe there's some transition period, and that's fine, but ultimately, Rails should just pick the best default choice and go with that. From my limited read on the situation, it seems like npm 5 is ultimately going to be that choice, but interested in hearing dissenting opinions on that.

…

On Tue, Jun 6, 2017 at 4:38 PM, Javan Makhmali ***@***.***> wrote: Big +1 for supporting npm in addition to yarn, but I think that attempting to do so automatically is going too far. As already seen from the comments, there are numerous versions and varying command arguments to consider. What do you think of making the package manager configurable in webpacker.yml? It could default to using yarn, and people can adjust as needed. Something like: # webpacker.ymldefault: &default package_manager: yarn package_add_args: add package_install_args: install — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKte_mnUSZX0GfpP3RjJAhC7KyyoHlks5sBWRZgaJpZM4NwpMt> .

ljharb · 2017-06-06T16:47:11Z

@dhh npm shrinkwrap was always sufficiently deterministic; it's just that its usability was unpleasant - you never needed to go with yarn in the first place, it was merely a valid choice for rails to make. (neither the yarn nor npm teams has ever provided a concrete scenario where "determinism" in npm 2-5 or yarn ever impacted users, assuming there was a lockfile present - I'm still waiting to see one)

If you only want to allow for one tool, npm is the proper one to allow since it ships with node. Artificially restricting it to npm 5+ to force users to have a lockfile is paternalistic, but fits with your stated goal. You could also just support all versions of npm, and mention in the docs that npm 5+ is preferred and why - that way users of node 8+ will get it automatically, and people who haven't upgraded node yet will have the option - but not the burden - of upgrading npm to 5+.

If you want to support yarn also, or allow rails to be configurable such that yarn users aren't forced to use npm, then that seems reasonable too - but imo that's quite tertiary to "npm" and "npm 5" support.

guilleiguaran · 2017-06-07T03:20:53Z

@ljharb we decided to choose Yarn over npm after of getting some feedback from rails users/contributors working in rails apps that had problems with npm and were pretty happy after of switching to Yarn (e.g Shopify). That's how we decide usually to adopt new technologies/standards/defaults/etc in Rails. In fact before of the release of Yarn we were very skeptic about supporting npm by default due to bad feedback and just the idea was discussed during a couple of years without reaching a clear consensus until the release of Yarn.

Asking to users for upgrades in new apps haven't been a problem so far (usually new versions of Rails require newer versions of Ruby). I think is ok to ask to users to use the latest version of npm.

I agree with @dhh on the idea of supporting both yarn/npm5+ during a transition period (maybe webpacker 2.x and Rails 5.2) but switching to npm5+ as unique default in the future (webpacker 3.0 and Rails 5.3/6.0).

Fix typos and cleanup Bump up npm version and add npm install to readme Add dev commands mapping Cleanup Move to private block

gdborton · 2017-06-14T19:31:38Z

It's probably fine that Rails only officially supports npm@5+, so long as it isn't stopping users from using < 5. Nothing about this PR so far is enforcing the version in code.

This is super helpful for teams that are stuck on older versions of npm or don't have a short term update path.

muryoimpl · 2017-06-28T06:22:14Z

CHANGELOG.md

+
+## Removed
+
+* Extra `ywebpacker:yarn_install` task - [#405](https://github.com/rails/webpacker/issues/405)


y is extra, I think 👀

dpogue · 2017-07-13T23:26:16Z

README.md

@@ -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+


Minor nit, but "npm" is always lowercase: https://twitter.com/seldo/status/850042251635867648

dpogue · 2017-07-13T23:28:19Z

README.md

@@ -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


npm >=5 will save by default, so --save is not required (4th bullet on the changelog for 5.0.0)

That's only true if save=false isn't in an .npmrc file somewhere, as many of us do who don't like npm 5+'s default autosave behavior. I think it's a good idea to keep it here.

justin808

@gauravtiwari and @dhh, if there could be looser coupling with the bin stub files and simply the specific of scripts to use, allowing the usage of node, yarn, or even wrapping those with https://www.npmjs.com/package/nps (as my team does), this looser coupling might provide a mechanism for flexibility (like the create-react-app "eject"), while enabling the core team to keep to just one omakase flavor. We had this issue early on in React on Rails where we tried to support too many things, such as including linter support in our installer.

justin808 · 2017-08-14T20:38:53Z

lib/webpacker/node_bundler.rb

+ npm.each do |package|
+ commands["#{package}"] = "#{package} install --save"
+ commands[:dev]["#{package}"] = "#{package} install --save-dev"
+ end


@gauravtiwari Will there be too much maintenance to support (every version of) npm and yarn.

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"

justin808 · 2017-08-14T20:41:36Z

lib/webpacker/node_bundler.rb

+
+ def available?(package)
+ null_device = Gem.win_platform? ? "/nul 2>&1" : "/dev/null 2>&1"
+ system("#{package} --version > #{null_device}")


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.

"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.

@justin808 The package manager is checked in order - yarn, npm (global) and then inside node_modules (local) - don't think using either affects the code so the env or config change isn't necessary. @ljharb true, unless of course they are already using it. NPM comes bundled as default with node so, thats "surely" available on all systems that has node.

I agree npm will generally always be available; I'm suggesting that the mere presence of yarn doesn't indicate a preference for it.

A yarn.lock file will though.

dhh · 2017-08-27T23:10:13Z

I'm not a fan of this two-horse approach. Webpacker is designed to work primarily with Rails 5.1+ and Rails has picked Yarn for now as the JavaScript packaging approach. That doesn't mean that someone couldn't go another route, but just that what we'll support out the box is Yarn. Now that we've extracted all logic from the tasks and moved them into the Webpacker class, it's trivially easy for someone who'd like to use a different approach to call the logic without relying on the tasks and prechecks we have.

ljharb · 2017-08-28T01:11:11Z

That's really unfortunate; especially since with the advent of npm 5 there's not really much reason to prefer yarn anyways.

poloka · 2017-09-27T17:31:52Z

Question then...
The existing logic for installing yarn is within an enhance to the recompilation of assets within the compile.rake. So if I'm rails < 5.x then yarn continues to install my javascript assets, how would I keep this from executing. I guess you can override the webpacker:yarn_install to do a npm install. Feels a little wrong.

I got a little crude and tried tying into the compile with a task to let the existing yarn installation go through and then remove the yarn.lock and perform a npm install.

if Rake::Task.task_defined?("webpacker:compile")
  Rake::Task["webpacker:compile"].enhance [:npm_over_yarn]
end

task :npm_over_yarn do
  require "FileUtils" unless defined?(FileUtils)

  package_lock =  File.expand_path("./package-lock.json")
  yarn_lock =  File.expand_path("./yarn.lock")

  if File.exists?(package_lock)
    $stdout.puts "Installing javascript dependencies using NPM"
    FileUtils.rm(yarn_lock) if File.exists?(yarn_lock)
    `npm install`
  else
    $stdout.puts "Installing javascript dependencies using YARN"
  end
end

It seems to work but pretty sure I'm not covering for what @dhh was thinking. ;-)

javan · 2017-09-27T19:15:36Z

FYI, npm >= 5.1.0 doesn't really do locking unless you specify exact versions in package.json: https://twitter.com/javan/status/905194268297441285

wickkidd · 2018-01-10T17:08:22Z

package-lock.json allegedly fixed in npm v5.4.2 npm/npm#17979 (comment)

swrobel · 2018-03-26T16:32:41Z

maybe consider this for webpacker 4? npm seems to be leaping ahead of yarn lately:
http://blog.npmjs.org/post/171556855892/introducing-npm-ci-for-faster-more-reliable

swrobel · 2018-03-29T05:39:58Z

Yet another reason to switch to npm:
yarnpkg/yarn#5467
yarnpkg/yarn#932

ytbryan · 2018-03-29T06:38:31Z

Actually, I hope that webpacker supports both package managers instead of switching from one to another? For example pass in an option like —npm or —yarn

And maybe it’s a good idea to do it in a separate PR

Use both npm and yarn

924d846

Use Rails.root Fix node_modules_path Fix text Prefix ruby path to executable

gauravtiwari requested review from javan and guilleiguaran June 5, 2017 22:30

ljharb suggested changes Jun 5, 2017

View reviewed changes

guilleiguaran reviewed Jun 6, 2017

View reviewed changes

guilleiguaran requested a review from dhh June 6, 2017 07:31

ljharb reviewed Jun 6, 2017

View reviewed changes

himdel reviewed Jun 6, 2017

View reviewed changes

Update changelog

f725437

Fix typos and cleanup Bump up npm version and add npm install to readme Add dev commands mapping Cleanup Move to private block

muryoimpl reviewed Jun 28, 2017

View reviewed changes

dpogue reviewed Jul 13, 2017

View reviewed changes

justin808 reviewed Aug 14, 2017

View reviewed changes

dhh closed this Aug 27, 2017

gauravtiwari mentioned this pull request Sep 26, 2017

package dependency resolution fails to use package-lock.json #865

Closed

jakeNiemiec mentioned this pull request Feb 20, 2020

Alternatives to Yarn #2467

Closed

jakeNiemiec mentioned this pull request Apr 30, 2020

Option to select package manager #1746

Closed


		## Removed

		* Extra `yarn:install` task - [#405](https://github.com/rails/webpacker/issues/405)


		## Removed

		* Extra `ywebpacker:yarn_install` task - [#405](https://github.com/rails/webpacker/issues/405)

Loosen up yarn and support npm #475

Loosen up yarn and support npm #475

Conversation

gauravtiwari commented Jun 5, 2017

Adds

Removes

gauravtiwari commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilleiguaran Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilleiguaran commented Jun 6, 2017

ljharb commented Jun 6, 2017

gauravtiwari commented Jun 6, 2017

guilleiguaran commented Jun 6, 2017 • edited Loading

ljharb commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincemtnz Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gargron commented Jun 6, 2017

himdel Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javan commented Jun 6, 2017

dhh commented Jun 6, 2017 via email

ljharb commented Jun 6, 2017

guilleiguaran commented Jun 7, 2017 • edited Loading

gdborton commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhh commented Aug 27, 2017

ljharb commented Aug 28, 2017

poloka commented Sep 27, 2017

javan commented Sep 27, 2017

wickkidd commented Jan 10, 2018

swrobel commented Mar 26, 2018

swrobel commented Mar 29, 2018

ytbryan commented Mar 29, 2018

guilleiguaran Jun 6, 2017 •

edited

Loading

guilleiguaran commented Jun 6, 2017 •

edited

Loading

vincemtnz Jun 6, 2017 •

edited

Loading

himdel Jun 6, 2017 •

edited

Loading

himdel Jun 7, 2017 •

edited

Loading

guilleiguaran commented Jun 7, 2017 •

edited

Loading