-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support React on Rails by merging Webpacker Lite #601
Support React on Rails by merging Webpacker Lite #601
Conversation
Gemfile
Outdated
end | ||
|
||
|
||
if ENV["USE_PRY"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove the pry stuff, but it's very useful for debugging.
f05e2f5
to
fe78242
Compare
lib/install/config/webpack/shared.js
Outdated
@@ -39,7 +38,6 @@ module.exports = { | |||
new webpack.EnvironmentPlugin(JSON.parse(JSON.stringify(env))), | |||
new ExtractTextPlugin(env.NODE_ENV === 'production' ? '[name]-[hash].css' : '[name].css'), | |||
new ManifestPlugin({ | |||
publicPath: output.publicPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gauravtiwari Can you help me with the webpack changes? I didn't see any docs on how to test this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gauravtiwari I created a test repo and tested the basic generator. What else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, will test with a new app 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gauravtiwari This is ready for review. My next step is that I'll release a beta gem with this for testing a beta of React on Rails v9.
Gemfile
Outdated
@@ -9,3 +9,14 @@ gem "rubocop", ">= 0.47", require: false | |||
group :test do | |||
gem "minitest", "~> 5.0" | |||
end | |||
|
|||
|
|||
if ENV["USE_PRY"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pry is super useful for debugging
lib/install/config/webpack/shared.js
Outdated
@@ -28,7 +28,6 @@ module.exports = { | |||
output: { | |||
filename: '[name].js', | |||
path: output.path, | |||
publicPath: output.publicPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should remove the subdirectory from the manifest.
lib/webpacker/configuration.rb
Outdated
def reset | ||
@defaults = nil | ||
super | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is used by tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is solely needed for testing, then we should just do it by hand in the tests. Not keen on having test-only methods part of the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/webpacker/configuration.rb
Outdated
def load | ||
return super unless File.exist?(@path) | ||
def load_data | ||
return Webpacker::Configuration.defaults unless File.exist?(@path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed load
to load_instance
and load_data
. Having just load
is super confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow? In which cases are we calling #load to mean different things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gauravtiwari fyi
@@ -3,31 +3,31 @@ | |||
class ConfigurationTest < Minitest::Test | |||
def test_entry_path | |||
entry_path = File.join(File.dirname(__FILE__), "test_app/app/javascript", "packs").to_s | |||
assert_equal Webpacker::Configuration.entry_path.to_s, entry_path | |||
assert_equal entry_path, Webpacker::Configuration.entry_path.to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI -- all tests wrote expected and actual in reverse order.
cf64c24
to
19bbb1e
Compare
@justin808 Hey, Sorry I am away until next week and don't have access to good internet connection :( Will take a look once I am back on 6th |
38657a1
to
e782dc3
Compare
See rails/webpacker#594 From a long discussion on #464, this issue will summarize. I'll soon be posting proposed changes to the README.md. Summary of changes * Move base url out from manifest.json to manifest.rb * Assign env variables to dev server settings so it can be overridden at runtime. * The keys for dev_server should use same format as of now as documented in Paths on the README.md. Note that * hot is a new setting to indicate that the dev_server is used with hot reloading, which means that CSS should be inlined to be hot loaded. The presence of dev_server means that the webpack-dev-server is used for the given env. development: // put the created files to the /public/webpack/development directory public_output_path: webpack/development //# if dev_server is not provided, then dev_server is not used dev_server: hot: true # This is a new setting static: false host: localhost https: false
e782dc3
to
afe9c99
Compare
@rafaelfranca @claudiob, any chance that either of you can take a look at this, given @gauravtiwari is unavailable? @gauravtiwari: I released shakacode/react_on_rails#908 as version 9.0.0.beta.1. |
@justin808 thanks for the PR. This is a big change and I'm not that familiar with the codebase of |
Since load_instance is a no-op for production, this should be fine performance-wise. If we're using `webpack -w` to recompile when files changes, we need to call this before find! Otherwise, we don't get new files. So if we startup both a compile process and the server with a Procfile, then Webpacker will load with no values in the manifest, and it will not be read again. * In the case of using
Live Reloading or Static Reloading? Live Reloading is having your assets for development provided by the webpack-dev-server. You can override the the presence of the `dev_server` setup by setting ENV value: `WEBPACKER_DEV_SERVER=FALSE`. You might do this if you are switching back and forth between statically compiling files during development and trying to get hot reloading to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great + explained well! I didn't see anything that seemed incorrect but left a couple questions which can be ignored if they don't make sense.
README.md
Outdated
@@ -1103,6 +1123,8 @@ git push heroku master | |||
Webpacker lazily compiles assets in test env so you can write your tests without any extra | |||
setup and everything will just work out of the box. | |||
|
|||
Note, [React on Rails] users should set configuration value `compile` to false, as React on Rails | |||
handle compilation for test and production environments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small grammar edit: handles
instead of handle
lib/webpacker/dev_server.rb
Outdated
end | ||
|
||
def https? | ||
fetch(:https) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does fetch(:https)
already return a boolean value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
lib/webpacker/manifest.rb
Outdated
end | ||
hashed_name = instance.data[name.to_s] | ||
return hashed_name if hashed_name.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's blank it might be ""
or nil
, does it matter which for the return value or should it be explicitly returning nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it would not matter.
test/helper_test.rb
Outdated
|
||
def test_stylesheet_pack_tag_outputs_nothing_for_hot | ||
Webpacker::DevServer.stub(:hot?, true) do | ||
# Webpacker::Configuration.reset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these commented out lines be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no -- I removed them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave to Pry and friends for another PR so we can't discuss their inclusion separately from the main purpose of this PR?
README.md
Outdated
@@ -601,8 +617,12 @@ and | |||
|
|||
#### React | |||
|
|||
You may consider using [react-rails](https://github.com/reactjs/react-rails) or | |||
[webpacker-react](https://github.com/renchap/webpacker-react) for more advanced react integration. However here is how you can do it yourself: | |||
The most widely used React integration is [shakacode/react_on_rails](https://github.com/shakacode/react_on_rails) which includes support for server rendering, redux and react-router. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just keep this to the facts: If you need more advanced React-integration, like server rendering, redux, or react-router, see [shakacode/react_on_rails](https://github.com/shakacode/react_on_rails), [react-rails](https://github.com/reactjs/react-rails), and [webpacker-react](https://github.com/renchap/webpacker-react)
.
lib/webpacker/dev_server.rb
Outdated
end | ||
|
||
private | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✂️ NL, indent everything below private.
Gemfile
Outdated
gem "pry-rails" | ||
gem "pry-rescue" | ||
gem "pry-stack_explorer" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need this as part of the package by default.
lib/webpacker/dev_server.rb
Outdated
|
||
class Webpacker::DevServer < Webpacker::FileLoader | ||
class << self | ||
def dev_server? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with enabled?
instead.
lib/webpacker/dev_server.rb
Outdated
return false if env_val == false | ||
|
||
# If not specified, then check if values in the config file for the dev_server key | ||
!dev_server_values.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can boil this down to:
def enabled?
case ENV["WEBPACKER_DEV_SERVER"]
when /true/i then true
when /false/i then false
else data.fetch(:dev_server).present?
end
end
lib/webpacker/dev_server.rb
Outdated
return false unless dev_server? | ||
env_val = env_value("WEBPACKER_HMR") | ||
return env_val unless env_val.nil? | ||
fetch(:hot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start by renaming to hot_reloading?
and then we can simplify the implementation:
if enabled?
case ENV["WEBPACKER_HMR"]
when /true/i then true
when /false/i then false
else fetch(:hot_reloading)
end
end
lib/webpacker/dev_server.rb
Outdated
return false if val_upcase == "FALSE" | ||
raise new ArgumentError("#{env_key} value is #{val}. Set to TRUE|FALSE") | ||
end | ||
# returns nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can nix this per the refactorings above.
lib/webpacker/dev_server.rb
Outdated
|
||
def dev_server_values | ||
data.fetch(:dev_server, nil) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nix this.
lib/webpacker/dev_server.rb
Outdated
|
||
def fetch(key) | ||
return nil unless dev_server? | ||
dev_server_values.fetch(key, dev_server_defaults[key]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use:
if enabled?
data[:dev_server][key] || Webpacker::Configuration.defaults[:dev_server][key]
end
lib/webpacker/dev_server.rb
Outdated
load_instance if Webpacker.env.development? | ||
unless instance | ||
raise Webpacker::FileLoader::FileLoaderError.new("Webpacker::DevServer.load_data must be called first") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same single-line format as the rest of the code base: raise Webpacker::FileLoader::FileLoaderError.new("Webpacker::DevServer.load must be called first") unless instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/webpacker/manifest.rb
Outdated
def asset_source(name) | ||
output_path_or_url = Webpacker::Configuration.output_path_or_url | ||
mapped_name = lookup(name) | ||
"#{output_path_or_url}/#{mapped_name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I don't think this is going to work when you have multiple directory outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yepp - the pack should have a full reference (if inside sub-directory)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gauravtiwari are you sure that the mapped_name
does not include the subdirectory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - yes that should work 👍 - webpack will reference sub-directories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh and @gauravtiwari I updated what I could in the code files. Regarding the directory and the service workers, I'd need to understand this case more before I can make any recommendations.
lib/webpacker/helper.rb
Outdated
asset_path(Webpacker::Manifest.lookup(name), **options) | ||
path = Webpacker::Configuration.public_output_path | ||
file = Webpacker::Manifest.lookup(name) | ||
full_path = "#{path}/#{file}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh We could configure using the same subdirectory of /public for the webpack-dev-server. Do you have any more details on your service worker example? Would there be new asset helpers?
lib/webpacker/dev_server.rb
Outdated
else fetch(:hot_reloading) | ||
end | ||
else | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh, your position is the same as our lead for https://www.friendsandguests.com, @robwise. However, rather than "on-demand", we prefer to have a separate watch process.
Why is on-demand preferable? Why not let webpack compile the files when you hit save rather than waiting for your browser to refresh?
As far as the React on Rails community, we've always done the combination of either HMR or static via webpack -w
, so we don't need the "live-reload" option.
One big problem with both live and hot reloading, per the documentation is that you don't always want the page to reload with every save as you might be experimenting with the browser styling and boom, accidental loss of your experimental changes.
If you now change any of the source files and save them, the web server will automatically reload after the code has been compiled. Give it a try!
Incidentally, the auto-reloading is not the default, per the documentation for the devServer.watchContentBase
configuration.
In favor of having the separate dev-server option, the documentation states for "Choosing a development tool":
In most cases, you probably would want to use webpack-dev-server, but let's explore all of the above options.
Of course, documentation is not always right!
lib/install/config/webpacker.yml
Outdated
@@ -33,7 +33,7 @@ development: | |||
host: localhost | |||
port: 8080 | |||
https: false | |||
hot_reloading: false | |||
hmr: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gauravtiwari @dhh FWIW, the docs for devServer.hot
use hot
and not hmr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think hmr
is consistent with names like 'WEBPACKER_HMR'
Also, considering usages like Webpacker::DevServer.hmr?
and def hmr?
, they are consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ytbryan I agree in the sense that DevServer.hot?
and WEBPACKER_HOT
are a bit ambiguous.
@gauravtiwari @dhh What's pending? I'd like to get this merged and a beta of the gem pushed so I can release 9.0.0 of React on Rails, per this PR: shakacode/react_on_rails#908 |
@justin808 Just finalising a final few things in regards to how to check if the dev-server is running. Basically, we want to make on-demand compilation as default so for ex - if the design team want to test something they don't have to run a dev server, it just works. However, if someone in the tech team wants to use the dev-server or watcher they can simply start the server or watcher, which would turn off on-demand compilation and everything should be served by dev-server, out of the box (no config changes required). |
@gauravtiwari Regarding your prior comment, please make no assumptions that the Rails wrappers around the executables are being used as many chose to run such executables directly from yarn scripts. Also, on-demand compilation might be from |
@justin808 Yepp, I know. |
Ofcourse folks are free to run either |
# Since load checks a `mtime` on the manifest for a non-production env before loading, | ||
# we should always call this before a call to `find!` since one may be using | ||
# `webpack -w` and a file may have been added to the manifest since Rails first started. | ||
load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not going to want to do a file stat in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh There's no file stat in production. The code checks the Rails.env. Does that meet your concern here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive the drive-by comment, but framework code should not be directly checking the environment: we have a config subsystem for that. People often have a production
-alike staging
environment, for example.
(More locally-relevant, this should likely use the file watcher to avoid the stat [where possible] even in development.)
lib/webpacker/dev_server.rb
Outdated
# Same convention as manifest/configuration.rb | ||
# Loads webpacker configuration from config/webpacker.yml | ||
|
||
class Webpacker::DevServer < Webpacker::Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to figure out what role this class plays that's separate from the configuration class. Most of the methods simply delegate. Is this just to overlay the ENV lookups? I don't think that's enough justification for the additional indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gauravtiwari, I think you'll need to comment here as you suggested this. I'm happy to merge these together.
We can't merge this in the current form where the manifest is changed from providing full paths to providing merely digest lookups, as we discussed earlier re: having different output paths, like with service workers. Also, I still don't follow the throw_if_missing business. The methods we're using, like find!, already imply a raise if missing. If you're wrapping these calls down the line, and you don't want the raise to make it to the user, can't you catch and log instead? |
From @dhh:
Should we change the API of The issue that I have with the lookup method always throw if the bundle name requested doesn't exist in the manifest is as follows. Possibly you can give me advice on particular use case that may eventually apply to Webpacker: In some cases but not always, we use a different Webpack configuration for a server bundle (server rendering). In this case, we'll typically run two Thus, for React on Rails on top of Webpacker, we'll have one of 2 scenarios:
The current code in React on Rails will first assume that all files produced by Webapck are in the manifest, and if not found, then the non-hashed, simple bundle-name is used Thus, for every single server request in production, we'd be doing a try/catch with the call to lookup as React on Rails is not tracking if the server bundle is or is not in the manifest. If we believe that these try/catches per request are unacceptable, possible solutions include:
Another case is that for test compilation when the test environment loads Webpacker, the manifest is empty, as React on Rails has not detected that the test run needs to compile the files. In this case, it's OK to catch the catch the exception and then compile. React on Rails will then run a task defined in sterr, stdout, status = Open3.capture3(webpack_env, "#{RbConfig.ruby} ./bin/webpack") React on Rails users have long used the conventional # If you are using the ReactOnRails::TestHelper.configure_rspec_to_compile_assets(config)
# with rspec then this controls what yarn command is run
# to automatically refresh your webpack assets on every test run.
config.npm_build_test_command = "yarn run build:test"
# This configures the script to run to build the production assets by webpack. Set this to nil
# if you don't want react_on_rails building this file for you.
config.npm_build_production_command = "yarn run build:production" While it might be feasible to force the React on Rails community to switch to using the |
@dhh wrote:
While I don't think there's a problem, besides some inconvenience, with putting back in the Doc references: |
I believe we've now extracted all the major parts of this PR into smaller changes adopted in master. We have the dev server running in a progressive enhancement mode where if you don't start it, we'll simply do on-demand compilation. This means testing just works with nothing else running, and for many smaller apps that'll be sufficient too. In your case, where you have some developers using the dev server and some using the watcher, all you'll need to do is turn off compilation. Then developers can run the dev server or the watcher as they please. We automatically detect a running dev server and will proxy to that if there. I've extracted the HMR enhancement into this PR: #641. Regarding the manifest for client and server side, I still don't fully understand your case. If you're using separate ways of generating these, and they have different expectations, why not just use two separate manifests? Re: service workers, I'm not quite sure yet. One option could simply be to start outputs in the root, and then have a manifest like { "service_worker.js" => "/service_worker-234234.js", "application.js" => "/packs/application-234234.js" }. We'll need to explore that as we do the proper implementation. So far we've removed one important roadblock, which is to proxy for the dev server, such that there are no same-origin violations any more. |
@dhh wrote:
In the case of using react-router, the server side and client side JS will be different (different entry points), and possibly some other build options. In some cases, one can use the same client side bundle, and thus that will be hashed in production, and the React on Rails code ideally will use the Webpacker function to read the manifest. If the manifest name is hard coded to be manifest.json, and the directory is set for the given ENV, then having 2 manifest files would require some API changes. I'm not convinced we need 2 manifests as we've not come across cases where the server needs more than one bundle. While some have asked for it, nobody has presented any compelling reason. Per my my prior comment above, the React on Rails server code might be able to avoid taking the hit of an exception every time the server bundle is missing from the manifest in non-development by simply detecting this case only once, and caching the result. For development, we can probably afford the extra catch for every request. @gauravtiwari and @dhh, I'm concerned about one case of turning off compilation and expecting watch mode to work. I can probably also look for the exception here. React on Rails supports a custom test build command that's run. Would you be open to the compilation supporting a custom command in the
|
@justin808 Let's try to see if the catch and cache case can work and take it from there 👍. Re: Compilation, that's no longer going through the rake tasks, but directly via Webpacker.compiler.compile. That's only for on-demand compilation, though. If you're using the watcher or the dev server, you can just start that with whatever command you like. So that should be good 👍 |
Going to close this PR now that the code is out of date and we've extracted all the major features and addressed the major concerns. Let's start smaller, feature-focused issues or PRs for whatever may be left. Thanks again for raising all these concerns. |
💥 React on Rails v9 fully supports v3 of Webpacker.Big thanks to the rails/webpacker team for getting in the necessary changes. 🙏 👏 |
See #594
PR to merge this into React on Rails: shakacode/react_on_rails#908
From a long discussion on #464, this issue will summarize. I'll soon be
posting proposed changes to the README.md.
Summary of changes
runtime.
in Paths on the README.md. Note that
reloading, which means that CSS should be inlined to be hot loaded.
The presence of dev_server means that the webpack-dev-server is used for
the given env.
development:
// put the created files to the /public/webpack/development directory
public_output_path: webpack/development
//# if dev_server is not provided, then dev_server is not used
dev_server:
hot: true # This is a new setting
static: false
host: localhost
https: false