Yarn: Move node_modules, package.json, and yarn.lock file to vendor #27245

Merged
merged 3 commits into from Dec 1, 2016

Projects

None yet

5 participants

@dhh
Member
dhh commented Dec 1, 2016

And then provide a binstub for yarn that runs in that directory.

@dhh dhh Move node_modules, package.json, and yarn.lock file to vendor
And then provide a binstub for yarn that runs in that directory.
5f3082e
@dhh dhh referenced this pull request Dec 1, 2016
Merged

Add Yarn support in new apps using --yarn option #26836

7 of 7 tasks complete
dhh added some commits Dec 1, 2016
@dhh dhh Don't use manually maintained vendor subdirectories when using yarn
We should drop the vendor CSS directory anyway, I think.
8012c11
@dhh dhh Tweak changelog to reflect updates
9ad3919
@dhh dhh merged commit 3dac36b into master Dec 1, 2016

0 of 3 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@dhh dhh deleted the yarn-from-vendor branch Dec 1, 2016
@@ -0,0 +1,2 @@
+VENDOR_PATH = File.expand_path('../vendor', __dir__)
+Dir.chdir(VENDOR_PATH) { exec "yarnpkg #{ARGV.join(" ")}" }
@ianks
ianks Dec 4, 2016 Contributor

Booting a whole ruby interpreter just to shell out to yarn seems unnecessary. This is especially painful for jruby folks.

@dhh
dhh Dec 4, 2016 Member

Booting Ruby isn't time consuming with MRI. But happy to take a patch with another approach that works on all platforms (and thus doesn't rely on Bash or whatever).

@ianks
ianks Dec 4, 2016 Contributor

FWIW, the yarn binstub relies on sh already.

@@ -1,7 +1,7 @@
-* Add Yarn support in new apps using --yarn option. This adds a package.json
+* Add Yarn support in new apps using --yarn option. This adds a yarn binstop, vendor/package.json,
@ianks
ianks Dec 4, 2016 Contributor

binstub*

@ianks ianks added a commit to ianks/rails that referenced this pull request Dec 4, 2016
@ianks ianks Use sh instead of Ruby script for yarn binstub
Seeing as yarn already uses sh for their binstub
(https://github.com/yarnpkg/yarn/blob/master/bin/yarn), we opt to use `sh` as
well. The main reason for this being that boot times for some interpreters
(i.e. jruby) is rather slow. This makes for a much better UX for these users.

See: rails#27245
676c614
@ianks ianks added a commit to ianks/rails that referenced this pull request Dec 4, 2016
@ianks ianks Use sh instead of Ruby script for yarn binstub
Seeing as yarn already uses sh for their binstub
(https://github.com/yarnpkg/yarn/blob/master/bin/yarn), we opt to use `sh` as
well. The main reason for this being that boot times for some interpreters
(i.e. jruby) is rather slow. This makes for a much better UX for these users.

See: rails#27245
c85debe
@lemonmade

I am really interested why this decision was made. It will make actually configuring any JavaScript tooling extremely difficult, and causes it to behave differently than it would in any other project that uses npm packages. As the simplest example, editor integration with a popular JavaScript linter like ESLint will not work anymore as they all rely on a "traditional" structure that has node_modules at the root, and that follows the basic node resolution algorithm.

At Shopify, we've had no trouble with node_modules at the root of the app (plus additional node_modules for separate bundles within the main application). It's been easier for new developers to understand where dependencies are coming from since it's just like any other npm-using project. If the only benefit is that the root directory is cleaned up a little bit, I don't see that as being worth the cost, particularly when other dependency information for the application (the Gemfile) is already stored at root.

@matthewd
Member
matthewd commented Dec 5, 2016

extremely difficult

@lemonmade can't you just create a top level node_modules directory if that works better for you? 😕

@dhh
Member
dhh commented Dec 5, 2016
@ianks
Contributor
ianks commented Dec 5, 2016

Having worked extensively with frontend tooling, I very much agree with @lemonmade. Pretty much all files that are in traditionally in will not be easily discoverable by tooling. It will end up just causing headaches for users by breaking conventions that already exist to create a new convention.

I think by default, node_modules and yarn.lock should be in the root directory. This way, the default experience will be painless as all node tooling can happily assume that executable will be in node_modules/.bin, etc. If someone wants move these to vendor, that should be fine; but IMO, don't cause integration pains for other users because you want a subjectively "cleaner" directory structure.

@dhh
Member
dhh commented Dec 5, 2016
@ianks
Contributor
ianks commented Dec 5, 2016 edited

As a counter-argument, convention over configuration is also a core value; which this implementation will invariably break. I do not think it is feasible to change the conventions of all the tooling around node to accommodate for this use case.

Here is what will happen in my case:

  1. My editor will look for a local version of mocha, tsc (typescript, or sass-lint inside of node_modules in the project root directory.
  2. When it does not find it, it will fall back to the global version of these commands
  3. These global versions will either:
    a. Not exist (best case)
    b. Exist, but potentially be a different version that what I have specified in my yarn.lock (worst case). Now, my editor is telling me things that are potentially wrong or outdated.
  4. This will cause me to waste my time attempting to reconfigure things/convincing the upstream editor to apply a patch which will fix my use case. None of this sounds like a pleasant experience (breaking core value number 1, Optimize for programmer happiness)
@dhh
Member
dhh commented Dec 5, 2016
@nathanmarks
nathanmarks commented Dec 6, 2016 edited

The issue here is that in order not to have a node_modules folder at the root (which doesn't get checked in anyways), conventions are being broken across the board.

If we're using a framework like rails to begin with, it stands to reason that we enjoy the comfortable conventions that the tools we use offer. I see no reason why rails has to break these JS conventions in order to accommodate JS, a critical component of web development.

  1. As @ianks mentioned, it'll mess with editor plugins and other related tools. Everything will need custom configuration (or potentially patches) just to work with Rails. Isn't convention over configuration a big part of The Rails Way?
  2. Take a simple feature such as Heroku buildpack inference, or CI inference -- they work on the basis that there is a package.json present in the project root. Point being... these are conventions that are respected across the board. All of a sudden, further configuration here is required to get started for the simplest use cases.
  3. Historically, (and not just in rails), /vendor folders were/are used to place 3rd party dependencies that are used in the application itself, not the dev tooling used to build the project. In fact, before package management was around for JS (bower, npm, etc), using a /vendor folder was also very common for non-rails projects.
  4. Unless I'm missing something, the only way to make node module resolution work for the /vendor/node_modules folder is either by:
    a. Using $NODE_PATH which is not recommended unless there's absolutely no other way. (Especially if it means that node module resolution is changed out of the box for anyone using rails for the sake of a folder that isn't checked into version control or controlled by rails... we're not talking about moving the assets/javascripts folder to the root or anything crazy)
    b. Monkey patching node's Module object at runtime
    c. Using relative requires to get things from the node_modules folder...

I'm loving that JS is getting some love in Rails, but my personal opinion is that this particular move is going to break the "Just Works" thing.

@dhh
Member
dhh commented Dec 7, 2016

@nathanmarks Thanks for your considerations. Good to have all this spelled out when we evaluate the current approach a bit further down the line. We're indeed using $NODE_PATH and I currently consider that to be a fully justified use. But as stated earlier: This isn't set in stone. We're going to try it for real and make it work, but if we can't, we'll reconsider. Let's resume this discussion in a month or two. In the mean time, eager to have everyone's help improving things.

#27288 is now available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment