Yarn: Move node_modules, package.json, and yarn.lock file to vendor #27245
Conversation
And then provide a binstub for yarn that runs in that directory.
We should drop the vendor CSS directory anyway, I think.
| @@ -0,0 +1,2 @@ | |||
| VENDOR_PATH = File.expand_path('../vendor', __dir__) | |||
| Dir.chdir(VENDOR_PATH) { exec "yarnpkg #{ARGV.join(" ")}" } | |||
There was a problem hiding this comment.
Booting a whole ruby interpreter just to shell out to yarn seems unnecessary. This is especially painful for jruby folks.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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, | |||
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
|
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 At Shopify, we've had no trouble with |
@lemonmade can't you just create a top level |
|
This isn't about whether node_modules + package.json + yarn.lock filling
into the root is "trouble" but whether it's a beautiful way to cohabit
app-focused JS within a Rails app. I don't think it is. As explored
earlier, I don't think that Gemfile* in the root is a good fit either. I'd
rather that both of those configurations lived in vendor/. And we'll work
to see if we can get there eventually too. But I'm not keen to compound the
problem with more vendor-configuration and install directories at our
already-busy root level.
We've found it pretty painless to configure both Yarn and Webpack to work
with this cohabitation structure. I'd be surprised if there weren't also
ways to make something like ESLint work as well.
Let's keep exploring.
…On Mon, Dec 5, 2016 at 12:47 PM, Chris Sauvé ***@***.***> wrote:
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.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#27245 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAKtd3FElALt6WrKgcifN0q-UeQqP82ks5rFHhugaJpZM4LBpdU>
.
|
|
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, |
|
Appreciate the input and will keep it under advisement as we go forward.
This isn't set in stone, but for now we're going to try to make this work.
Also, beautiful code and structures may well be "subjective", but it's a
core value for Rails (see http://rubyonrails.org/doctrine/#beautiful-code
for more). So let's try to make this work. If we put in all the effort and
its still a bust in the end, we can take a different path from there.
…On Mon, Dec 5, 2016 at 1:37 PM, Ian Ker-Seymer ***@***.***> wrote:
Having worked extensively with frontend tooling, I very much agree with
@lemonmade <https://github.com/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.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#27245 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAKtc7GNflmDk6vs02QepYRE1Xkt4G8ks5rFIQBgaJpZM4LBpdU>
.
|
|
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:
|
|
I understand That's Just The Way Things Are but I always mentally
compliment that with "for the moment" and "doing things as we do". The
birth of this project was to push back against those things. Work with
others to change things and integrate them in ways for mutual benefit. I'd
like to keep trying to do that here.
…On Mon, Dec 5, 2016 at 2:09 PM, Ian Ker-Seymer ***@***.***> wrote:
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.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#27245 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAKtSDs1XZlB4W5F0JbJ-VYu4dCrU2eks5rFIuygaJpZM4LBpdU>
.
|
|
The issue here is that in order not to have a 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.
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. |
|
@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. |
|
For what it's worth, it might be possible to add the option to set the The maintainers seem open to it. |
|
In the end, David set it to the root Rails application folder in #28093. |
|
Is there any documentation on whether |
And then provide a binstub for yarn that runs in that directory.