Skip to content

Conversation

dleavitt
Copy link
Contributor

@dleavitt dleavitt commented Feb 6, 2017

I was reading through rails/rails#27245, and thought the best and most "Railsey" solution would be to use vendor by default but make other options easy as well.

This PR adds a new rake task, webpacker:install:root, which uses Rails.root as the root directory for yarn, installing node_modules, package.json and yarn.lock there.

webpacker:install continues to function as it always has, installing to vendor.

This change is backwards compatible. It just adds some conditionals to the install template; none of the runtime or asset compilation code is affected. All of that as well as all of the instructions in the README still just work, whether you install to vendor or to root.

I think this is a good solution in that it leaves vendor as the default but puts the choice in the hands of early adopters, allowing you guys to gather feedback on both approaches.

Why it's important to have this choice

I know this has already been discussed, but I want to reiterate why it's important to have this as an option. The Javascript ecosystem has matured, and having package.json and node_modules in the project root (or at least above all JS code) is one of its few universals. Because of this:

  • Using vendor breaks most existing tooling. It can be worked around in some cases, but not in others. E.g. I don't think there's any way at the moment to get Atom's eslint package to handle this without major nastiness. Bundling one-off solutions for every popular use case (eg third party tools blowing up trying to find package.json and node_modules #41) is not a great solution.
  • Using vendor breaks package.json scripting, JS land's bundle exec / rake / binstubs. Instead JS commands have to be wrapped in bin scripts or rake tasks that call out to Javascript. This is gross to implement and especially painful on JRuby. Wouldn't it be nice just to run yarn start to boot up the frontend?
  • If you're using webpack, your frontend is likely just as serious as your backend. Using vendor marginalizes the frontend.
  • Using vendor raises the barrier to getting Javascript developers excited about Rails. Their tooling doesn't work, their conventions don't apply, they need to edit obscure Ruby files to do basic things they could otherwise do straightforwardly in JS. Rails has gotten a reputation as being behind the curve on modern frontend development. This won't help.

Other notes / topics for bikeshedding

  • There aren't tests for this repo yet, so I made a little fixture app here to demonstrate that this works: https://github.com/dleavitt/webpacker-testcase
  • Is webpacker:install:root the cleanest way to do this? Should it be a flag or something instead?
  • Not sure what the best practice is for passing info to the .tt files. Currently just using an ENV var.
  • Not sure if my variable names are the best.
  • webpacker:install:angular won’t quite work out of the box after webpacker:install:root - a couple paths will need to be adjusted in the generated tsconfig file.
  • This will probably necessitate corresponding changes to Rails’s yarn generator.

In summary:

61790976

dleavitt and others added 2 commits February 5, 2017 19:26
This PR adds a new rake task, `webpacker:install:root`, which uses `Rails.root` as the root directory for yarn, installing `node_modules, `package.json`, and `yarn.lock` there.

`webpacker:install` continues to function as it always has, installing to `vendor`.

This only touches the install template - none of the runtime or asset compilation code is affected.
@pstjean
Copy link

pstjean commented Feb 17, 2017

I want to chime in to agree on the subject of vendor breaking existing tooling. Even very mature and well maintained projects treat projects without package.json and node_modules in the root as second class citizens.

I spent the day yesterday configuring Jest testing for a vanilla Rails and Webpacker setup, after seeing a week old, unanswered question on Stack Overflow inquiring how to get JavaScript testing (of any description) working alongside Webpacker. I figured it couldn't be that hard. I was completely wrong.

I wrestled with documentation in both the Jest and Babel projects that was self-contradictory and outdated. We can self-righteously condemn the maintainers of these projects for inadequate documentation, but the simple fact of the matter is that only a very tiny minority of their users ever use their product in a project that doesn't follow the convention of placing package.json and node_modules in the project root. They don't prioritize these users, and they can't be expected to.

If this is the situation with two extremely well maintained and highly regarded projects, it's easy to see why a lot of smaller Node projects simply omit support for non-standard configurations like the one implemented by Webpacker. We'll be left in a situation where we're forking JavaScript libraries to support the peculiar Rails way of handling JavaScript dependencies, and at that point we're right back at JavaScript gems.

@dhh
Copy link
Member

dhh commented Feb 21, 2017

Going back to root for everything. Please do update this PR just assume the root 👍

@dleavitt
Copy link
Contributor Author

Will do! Thanks!

@dleavitt
Copy link
Contributor Author

@guilleiguaran @dhh closing this, updated pr is #101

@dleavitt dleavitt closed this Feb 21, 2017
claudiob added a commit to claudiob/webpacker that referenced this pull request Feb 22, 2017
Closes rails#102

As a result of rails#84 and of rails/rails#28093, generating a new rails app
with webpacker was raising a conflict in the creation of the `bin/yarn`
file. This commit removes that conflict.
claudiob added a commit to claudiob/webpacker that referenced this pull request Feb 22, 2017
Closes rails#102

As a result of rails#84 and of rails/rails#28093, generating a new rails app
with webpacker was raising a conflict in the creation of the `bin/yarn`
file. This commit removes that conflict.
dhh pushed a commit that referenced this pull request Feb 22, 2017
Closes #102

As a result of #84 and of rails/rails#28093, generating a new rails app
with webpacker was raising a conflict in the creation of the `bin/yarn`
file. This commit removes that conflict.
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.

3 participants