Binstubs #833
Binstubs #833
Conversation
|
thanks @rossta ❤️ 💯 for the PR Looks like rubocop isn't letting travis pass so if you can fix the offences, I will merge 👍 |
lib/webpacker/runner.rb
Outdated
| def initialize(argv) | ||
| @argv = argv | ||
|
|
||
| @app_path = File.expand_path("../", __dir__) |
There was a problem hiding this comment.
Does __dir__ work correctly here (in a file in the gem)?
There was a problem hiding this comment.
No, good catch—error in local testing. I may need to pass the app_path into the runner which may mean keeping the install template with less code instead of using a bundler binstub.
lib/webpacker/runner.rb
Outdated
| require "shellwords" | ||
|
|
||
| module Webpacker | ||
| class Runner |
There was a problem hiding this comment.
Perhaps Runner should be the base class that all runner classes inherit from.
There was a problem hiding this comment.
Like:
# lib/webpacker/runners/webpack.rb
class Webpacker::WebpackRunner < Webpacker::Runner
# lib/webpacker/runners/dev_server.rb
class Webpacker::DevServerRunner < Webpacker::Runner
lib/webpacker/dev_server_runner.rb
Outdated
| exit! | ||
| end | ||
|
|
||
| def update_argv |
There was a problem hiding this comment.
We need this empty method here?
lib/webpacker/dev_server_runner.rb
Outdated
| @app_path = File.expand_path("../", __dir__) | ||
| @config_file = File.join(@app_path, "config/webpacker.yml") | ||
| @node_modules_path = File.join(@app_path, "node_modules") | ||
| @webpack_config = File.join(@app_path, "config/webpack/#{ENV["NODE_ENV"]}.js") |
There was a problem hiding this comment.
May be we can load options from Webpacker.config or dev_server itself instead of loading the yml again?
There was a problem hiding this comment.
It would be great to avoid that duplication. Currently, Webpacker.dev_server inherits form Webpacker::Instance which depends on Rails (for Rails.root here) and I think it would be preferable to avoid depending on Rails to execute these scripts. Perhaps a future improvement?
| def initialize(argv) | ||
| @argv = argv | ||
|
|
||
| @app_path = File.expand_path(".", Dir.pwd) |
There was a problem hiding this comment.
Using Dir.pwd has pros and cons: we can install the executables as binstubs with bundler (which won't supply the application dir context) but we wouldn't be able to execute the binstubs from anywhere but the project root. I'm unsure of how this might affect various deployment scenarios.
There was a problem hiding this comment.
Yeah... maybe you can leverage the root_path accessor set via webpacker itself which defaults to Rails.root? Not sure what's best in this context?
There was a problem hiding this comment.
Rails.root would make it easy but I was hoping to avoid relying on loading rails since these are just thin wrappers around node executables.
There was a problem hiding this comment.
Some alternatives:
- Re-instate the bin templates (backing out of the the bundler binstub change) so we can pass in the app path to the runner
- Support an ENV var like
APP_PATHto override the default
There was a problem hiding this comment.
Yeah, lets tackle that in another PR
|
I've created a test repo pointing to this branch to demonstrate behavior with the new binstubs: https://github.com/rossta/webpacker_test |
| def initialize(argv) | ||
| @argv = argv | ||
|
|
||
| @app_path = File.expand_path(".", Dir.pwd) |
There was a problem hiding this comment.
Yeah... maybe you can leverage the root_path accessor set via webpacker itself which defaults to Rails.root? Not sure what's best in this context?
lib/webpacker/dev_server_runner.rb
Outdated
| class DevServerRunner < Webpacker::Runner | ||
| def run | ||
| load_config | ||
| check_server! |
There was a problem hiding this comment.
may be call this detect_port!?
|
@rossta Ready to merge? Looks good to me |
|
@gauravtiwari Ready! |
|
Merged 🎉 🍰 |
Converts
bin/webpack-dev-serverandbin/webpackto binstubs. This convention would make it easier to change the implementation of the two task runners without requiring devs to re-run the installer.I'd expect it would be simple to merge changes from the configurable-dev-server-options branch.
Fixes #831