React on Rails: last issue is the check for the binstubs crashes if the binstub does not exist #656
Conversation
|
@dhh @gauravtiwari I placed my one commit on top of master. Please review. |
lib/webpacker/manifest.rb
Outdated
| end | ||
| end | ||
|
|
||
| def load | ||
| if config.public_manifest_path.exist? | ||
| if config.public_manifest_path.exist? && | ||
| (@parsed_mtime.nil? || |
There was a problem hiding this comment.
@justin808 What this does? We are already using cached timestamp
There was a problem hiding this comment.
This makes sure we don't re-parse the JSON manifest every time for the helper. The cached timestamp is for re-compilation. React on Rails is not using the compile feature.
There was a problem hiding this comment.
Not sure I follow. In production, we'll shortly have cache_manifest, so there won't be any performance penalty. In development and test, I'd be surprised if there's any big performance ding from parsing the JSON on demand as needed? This isn't going to be a huge multi-megabyte file.
justin808
force-pushed
the
issue-464-merge-webpacker-lite-into-webpacker-v3
branch
from
dfff9de
to
89a59a3
Compare
| @@ -5,6 +5,7 @@ default: &default | |||
| source_entry_path: packs | |||
| public_output_path: packs | |||
| cache_path: tmp/cache/webpacker | |||
| custom_compile: false | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
| 5. Your Webpack configuration is not creating a manifest. | ||
| Your manifest contains: | ||
| #{@data.to_json} | ||
| MSG |
There was a problem hiding this comment.
@gauravtiwari @dhh Do we want a more detailed error message? or at least what I did on line 45 to output the manifest?
| $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? |
There was a problem hiding this comment.
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 👍
This fix addresses the cases where compile is disabled: 1. If React on Rails depends on Webpacker, but the React on Rails installer has not yet generated a webpacker.yml file or a React on Rails installation is not using Webpacker, then the gem loading of Webpacker will crash due to the missing config file. 2. If React on Rails has its own helper to compile assets for tests, then the manifest might need to get reloaded after Webpacker statically loads.
Provide an easy way to tell webpacker that we have a custom compile operation, such as used by react on rails.
justin808
force-pushed
the
issue-464-merge-webpacker-lite-into-webpacker-v3
branch
from
89a59a3
to
ab4eb01
Compare
| @@ -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")) | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
| @@ -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? | |||
There was a problem hiding this comment.
The :check_binstubs is the issue, not the part below.
justin808
changed the title
|
We have 3 PR's open for same thing :) |
Small changes for React on Rails based on master branch of rails/webpacker
I can move the better error message part to a separate PR tomorrow.
The only issue is the check for the binstubs crashes if the binstub does not exist.
I have updated shakacode/react_on_rails#908