Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React on Rails: last issue is the check for the binstubs crashes if the binstub does not exist #656

Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/install/config/webpacker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ default: &default
source_entry_path: packs
public_output_path: packs
cache_path: tmp/cache/webpacker
custom_compile: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhh @gauravtiwari I need to add something like this so that assets:precompile does not try to use the bin/webpacker command.

We can't just turn compile off, as that's used to determine if compile checking should happen. Maybe instead of true/false, we could have a third value of "custom"? Whatever you prefer is fine with me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand? Should be possible for React on Rails to overwrite the webpacker:compile task or to even remove it from the assets:precompile preconditions. I think that's probably the better move.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhh Right now, the default precompile from both Webpacker and React on Rails is getting run. Rake is additive. I don't see a good way for React on Rails to disable to the Webpacker task. That's all I want to do.


# Additional paths webpack should lookup modules
# ['app/assets', 'engine/foo/app/assets']
Expand Down
3 changes: 2 additions & 1 deletion lib/tasks/webpacker/check_binstubs.rake
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
namespace :webpacker do
desc "Verifies that bin/webpack & bin/webpack-dev-server are present."
task :check_binstubs do
unless File.exist?("bin/webpack") && File.exist?("bin/webpack-dev-server")
unless Webpacker.config.custom_compile? ||
(File.exist?("bin/webpack") && File.exist?("bin/webpack-dev-server"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhh @gauravtiwari How do we work around this part for React on Rails users that will not want to have the bin/webpack file installed?

It turns out that if the automatic precompile task runs, this message prints:

No compiling needed as everything is fresh

as React on Rails first does the compile, so this seems OK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justin808 Yeah probably should test for config/webpacker.yml since that's more primary than checking binstubs.

We discussed to remove logger output - don't think it's needed when assets are fresh.

$stderr.puts "Webpack binstubs not found.\n"\
"Have you run rails webpacker:install ?\n"\
"Make sure the bin directory or binstubs are not included in .gitignore\n"\
Expand Down
14 changes: 8 additions & 6 deletions lib/tasks/webpacker/verify_install.rake
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@ require "webpacker/configuration"
namespace :webpacker do
desc "Verifies if webpacker is installed"
task verify_install: [:check_node, :check_yarn, :check_binstubs] do
if Webpacker.config.config_path.exist?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The :check_binstubs is the issue, not the part below.

$stdout.puts "Webpacker is installed 🎉 🍰"
$stdout.puts "Using #{Webpacker.config.config_path} file for setting up webpack paths"
else
$stderr.puts "Configuration config/webpacker.yml file not found. \n"\
unless Webpacker.config.custom_compile?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we've extracted all the logic from the rake tasks into the Webpacker classes, I think the better move here, rather than trying to hack the webpacker tasks, is probably just to have your own reactonrails:* tasks that call these Webpacker classes directly. Then you can tailor output and prerequisites as you please 👍

if Webpacker.config.config_path.exist?
$stdout.puts "Webpacker is installed 🎉 🍰"
$stdout.puts "Using #{Webpacker.config.config_path} file for setting up webpack paths"
else
$stderr.puts "Configuration config/webpacker.yml file not found. \n"\
"Make sure webpacker:install is run successfully before " \
"running dependent tasks"
exit!
exit!
end
end
end
end
4 changes: 4 additions & 0 deletions lib/webpacker/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ def compile?
fetch(:compile)
end

def custom_compile?
fetch(:custom_compile)
end

def source_path
root_path.join(fetch(:source_path))
end
Expand Down
16 changes: 15 additions & 1 deletion lib/webpacker/manifest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,21 @@ def find(name)

def handle_missing_entry(name)
raise Webpacker::Manifest::MissingEntryError,
"Can't find #{name} in #{config.public_manifest_path}. Is webpack still compiling?"
"Can't find #{name} in #{config.public_manifest_path}. Manifest contains:\n#{JSON.pretty_generate(@data)}"
end

def missing_file_from_manifest_error(bundle_name)
msg = <<-MSG
Webpacker can't find #{bundle_name} in #{config.public_manifest_path}. Possible causes:
1. You are hot reloading.
2. You want to set Configuration.compile to true for your environment.
3. Webpack has not re-run to reflect updates.
4. You have misconfigured Webpacker's config/webpacker.yml file.
5. Your Webpack configuration is not creating a manifest.
Your manifest contains:
#{@data.to_json}
MSG
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gauravtiwari @dhh Do we want a more detailed error message? or at least what I did on line 45 to output the manifest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good 👍

raise(Webpacker::FileLoader::NotFoundError.new(msg))
end

def data
Expand Down
11 changes: 10 additions & 1 deletion test/manifest_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,17 @@ def test_lookup_exception
error = assert_raises Webpacker::Manifest::MissingEntryError do
Webpacker.manifest.lookup(asset_file)
end
expected = <<-MSG.strip
Can't find calendar.js in #{manifest_path}. Manifest contains:
{
"bootstrap.css": "/packs/bootstrap-c38deda30895059837cf.css",
"application.css": "/packs/application-dd6b1cd38bfa093df600.css",
"bootstrap.js": "/packs/bootstrap-300631c4f0e0f9c865bc.js",
"application.js": "/packs/application-k344a6d59eef8632c9d1.js"
}
MSG

assert_equal "Can't find #{asset_file} in #{manifest_path}. Is webpack still compiling?", error.message
assert_equal expected, error.message
end
end

Expand Down