-
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
Changes from 8 commits
afe9c99
41cda6a
b73fa8e
c71bd6f
4cf3fca
23c1f23
4498a48
5740921
32ff0d0
5599fd6
bb71faa
96fa67e
fe292e6
e4337db
d523fb9
007e36e
7f7fcac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,4 +134,4 @@ DEPENDENCIES | |
webpacker! | ||
|
||
BUNDLED WITH | ||
1.14.6 | ||
1.15.3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -367,8 +367,14 @@ development: | |
host: 0.0.0.0 | ||
port: 8080 | ||
https: false | ||
hot_reloading: false | ||
``` | ||
|
||
If you have hot_reloading turned to `true`, then the `stylesheet_pack_tag` generates no output, as you will want | ||
to configure your styles to be inlined in your JavaScript for hot reloading. During production and testing, the | ||
`stylesheet_pack_tag` will create the appropriate HTML tags. | ||
|
||
|
||
#### Resolved Paths | ||
|
||
If you are adding webpacker to an existing app that has most of the assets inside | ||
|
@@ -453,6 +459,16 @@ You can checkout these links on this subject: | |
- https://webpack.js.org/configuration/dev-server/#devserver-hot | ||
- https://webpack.js.org/guides/hmr-react/ | ||
|
||
If you are using hot reloading, you should either set the `dev_server/hot_reloading` key to TRUE or set the | ||
ENV value `WEBPACKER_HMR=TRUE`. That way, your stylesheet pack tag will do **nothing** because you | ||
need your styles inlined in your JavaScript for hot reloading to work properly. | ||
|
||
### 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. | ||
|
||
## Linking Styles, Images and Fonts | ||
|
||
|
@@ -601,8 +617,9 @@ 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: | ||
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). | ||
|
||
If you're not concerned with view helpers to pass props or server rendering, can do it yourself: | ||
|
||
```erb | ||
<%# views/layouts/application.html.erb %> | ||
|
@@ -1103,6 +1120,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 | ||
handles compilation for test and production environments. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think all integration advice for React on Rails is best presented within the React on Rails documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
Here is a sample system test case with hello_react example component: | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,8 +27,7 @@ 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 commentThe reason will be displayed to describe this comment to others. Learn more. This should remove the subdirectory from the manifest. |
||
path: output.path | ||
}, | ||
|
||
module: { | ||
|
@@ -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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Looks good to me, will test with a new app 👍 |
||
writeToFileEmit: true | ||
}) | ||
], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ development: | |
host: localhost | ||
port: 8080 | ||
https: false | ||
hot_reloading: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dhh, per the method change from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or perhaps we call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gauravtiwari @dhh Let's see if we can come to a quick agreement on the naming of the ENV and YML values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with HMR as well, though that raises a larger question. Now that we have lazy compilation available without a separate dev server, isn't HMR the only reason why you'd use the dev server? If so, maybe it should be called the webpack-hmr-server and focus on that task exclusively. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't that point even further to the fact that the dev server should just be relegated to the live-reloading/hmr option? If you have team members who don't want/need that, then they can just rely on on-demand compilation, which is the default now anyway? |
||
|
||
test: | ||
<<: *default | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,15 @@ module Webpacker | |
extend self | ||
|
||
def bootstrap | ||
Webpacker::Env.load | ||
Webpacker::Configuration.load | ||
Webpacker::Manifest.load | ||
Webpacker::Env.load_instance | ||
Webpacker::Configuration.load_instance | ||
Webpacker::DevServer.load_instance | ||
Webpacker::Manifest.load_instance | ||
end | ||
|
||
def compile | ||
Webpacker::Compiler.compile | ||
Webpacker::Manifest.load | ||
Webpacker::Manifest.load_instance | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry but could we please drop these changes and move to another PR that way we can merge this one quickly and then address the naming issue in another PR :) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which is "this one"? If we can keep this small refactoring in here, I'd prefer it. @dhh? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I meant, change from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Concur with @gauravtiwari on this being unrelated to this PR. I also don't think it's an improvement but we could discuss that in another ticket. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dhh and @gauravtiwari if you want me to rename all the load_instance and load_data to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert all back to #load, yes. We can discuss a change elsewhere. |
||
end | ||
|
||
def env | ||
|
@@ -19,6 +20,7 @@ def env | |
|
||
require "webpacker/env" | ||
require "webpacker/configuration" | ||
require "webpacker/dev_server" | ||
require "webpacker/manifest" | ||
require "webpacker/compiler" | ||
require "webpacker/railtie" if defined?(Rails) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,12 @@ def entry_path | |
source_path.join(fetch(:source_entry_path)) | ||
end | ||
|
||
def public_output_path | ||
fetch(:public_output_path) | ||
end | ||
|
||
def output_path | ||
public_path.join(fetch(:public_output_path)) | ||
public_path.join(public_output_path) | ||
end | ||
|
||
def manifest_path | ||
|
@@ -45,19 +49,30 @@ def fetch(key) | |
end | ||
|
||
def data | ||
load if Webpacker.env.development? | ||
raise Webpacker::FileLoader::FileLoaderError.new("Webpacker::Configuration.load must be called first") unless instance | ||
load_instance if Webpacker.env.development? | ||
raise Webpacker::FileLoader::FileLoaderError.new("Webpacker::Configuration.load_data must be called first") unless instance | ||
instance.data | ||
end | ||
|
||
def defaults | ||
@defaults ||= HashWithIndifferentAccess.new(YAML.load(default_file_path.read)[Webpacker.env]) | ||
end | ||
|
||
# Uses the webpack dev server host if appropriate | ||
def output_path_or_url | ||
if Webpacker::DevServer.enabled? | ||
Webpacker::DevServer.base_url | ||
else | ||
# Ensure we start with a slash so that the asset helpers don't prepend the default asset | ||
# pipeline locations. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just require that in the configuration file? Seems a bit defensive otherwise to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dhh would you prefer an exception if the path does not begin with a slash? # Uses the webpack dev server host if appropriate
def output_path_or_url
if Webpacker::DevServer.enabled?
Webpacker::DevServer.base_url
else
# Ensure we start with a slash so that the asset helpers don't prepend the default asset
# pipeline locations.
public_output_path.starts_with?("/") ? public_output_path : "/#{public_output_path}"
end
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need any particular guard there. We'll birth the file with the right defaults. If people from there remove the first slash, they'll get whatever that gives. |
||
public_output_path.starts_with?("/") ? public_output_path : "/#{public_output_path}" | ||
end | ||
end | ||
end | ||
|
||
private | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I changed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @gauravtiwari fyi |
||
HashWithIndifferentAccess.new(YAML.load(File.read(@path))[Webpacker.env]) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
# Same convention as manifest/configuration.rb | ||
|
||
# Loads webpacker configuration from config/webpacker.yml | ||
|
||
require "webpacker/configuration" | ||
|
||
class Webpacker::DevServer < Webpacker::FileLoader | ||
class << self | ||
def enabled? | ||
case ENV["WEBPACKER_DEV_SERVER"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we reverse the behaviour with env variable - like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK with any way to solve the problem of having some developers on my team have to keep a modified version of the yml file.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the use case for locally forking the configuration file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comments below on how different team members want "live-reloading" or just the statically built files. |
||
when /true/i then true | ||
when /false/i then false | ||
else !data[:dev_server].nil? | ||
end | ||
end | ||
|
||
# read settings for dev_server | ||
def hot_reloading? | ||
if enabled? | ||
case ENV["WEBPACKER_HMR"] | ||
when /true/i then true | ||
when /false/i then false | ||
else fetch(:hot_reloading) | ||
end | ||
else | ||
false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above ENV['DISABLE_HMR'] || fetch(:hot_reloading) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current idea is that the hot reloading setting only applies if the dev server is enabled. An alternative interpretation could be that the ENV for hot reloading overrides the dev server setting. I think that's getting too complicated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gets even closer to the point that perhaps the dev server should just focus exclusively on HMR now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dhh @gauravtiwari My team at https://www.friendsandguests.com has been doing static files for development mostly, with a few developers trying hot-reloading. We never explored any possible speed differences with doing just "live-reloading" (which is using the webpack-dev-server without HMR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, as above, I think we should refocus this. The dev-server is there when you want live-reloading (and potentially HMR). On-demand compilation, the new default, is there when . you just want "static files" (which, coincidentally, is where we ended up at Basecamp as well -- we didn't like live reloading either). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Incidentally, the auto-reloading is not the default, per the documentation for the In favor of having the separate dev-server option, the documentation states for "Choosing a development tool":
Of course, documentation is not always right! |
||
end | ||
end | ||
|
||
def host | ||
fetch(:host) | ||
end | ||
|
||
def port | ||
fetch(:port) | ||
end | ||
|
||
def https? | ||
fetch(:https) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
end | ||
|
||
def protocol | ||
https? ? "https" : "http" | ||
end | ||
|
||
def file_path | ||
Webpacker::Configuration.file_path | ||
end | ||
|
||
# Uses the hot_reloading_host if appropriate | ||
def base_url | ||
"#{protocol}://#{host}:#{port}" | ||
end | ||
|
||
private | ||
def fetch(key) | ||
if enabled? | ||
data[:dev_server][key] || Webpacker::Configuration.defaults[:dev_server][key] | ||
end | ||
end | ||
|
||
def data | ||
load_instance if Webpacker.env.development? | ||
raise Webpacker::FileLoader::FileLoaderError.new("Webpacker::DevServer.load_data must be called first") unless instance | ||
instance.data | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
end | ||
|
||
private | ||
def load_data | ||
Webpacker::Configuration.instance.data | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,21 +4,38 @@ class NotFoundError < StandardError; end | |
class FileLoaderError < StandardError; end | ||
|
||
class_attribute :instance | ||
attr_accessor :data | ||
attr_accessor :data, :mtime, :path | ||
|
||
class << self | ||
def load(path = file_path) | ||
self.instance = new(path) | ||
def load_instance(path = file_path) | ||
if instance.nil? || !production_env? || !file_cached?(path) | ||
self.instance = new(path) | ||
end | ||
end | ||
|
||
def file_path | ||
raise FileLoaderError.new("Subclass of Webpacker::FileLoader should override this method") | ||
end | ||
|
||
private | ||
def file_cached?(path) | ||
File.exist?(path) && self.instance.mtime == File.mtime(path) | ||
end | ||
|
||
# Prefer the NODE_ENV to the rails env. | ||
def production_env? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
so if we just call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gauravtiwari are you sure that would work since you're creating a circular dependency amongst the files. I don't think so but if you're sure, I'll try. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yes, totally missed 👍 |
||
(ENV["NODE_ENV"].presence || Rails.env) == "production" | ||
end | ||
end | ||
|
||
private | ||
def initialize(path) | ||
@path = path | ||
@data = load | ||
@mtime = File.exist?(path) ? File.mtime(path) : nil | ||
@data = load_data | ||
end | ||
|
||
def load | ||
def load_data | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it very confusing to have |
||
{}.freeze | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,12 @@ module Webpacker::Helper | |
# In production mode: | ||
# <%= asset_pack_path 'calendar.css' %> # => "/packs/calendar-1016838bab065ae1e122.css" | ||
def asset_pack_path(name, **options) | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Why wouldn't we have all this work happening in the manifest? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dhh The manifest should not know about where the file is on the Rails server. When in webpack-dev-server mode, there is typically no subdirectory. In other words. Let's have the manifest be a simple mapping of the bundle name to the possibly hashed version of the bundle name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow. What's the advantage of having the dev-server use a different path than what'll be used in production? That doesn't seem like a win to me. Also, we're going to have to support multiple paths in the future to deal with service workers, as they're scoped by the directory structure. So you could have /packs/calendar.css and /service_worker.js both be generated. Can't disambiguate between the two unless the path is part of it. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've only just started working on the service worker angle, but it's clear from the early exploration that it won't work well to force it inside /packs/. Maybe there are other solutions to the issue, but I'd like to understand more about why the manifest couldn't just use full paths? This isn't just a digest index. |
||
asset_path(full_path, **options) | ||
end | ||
|
||
# Creates a script tag that references the named pack file, as compiled by Webpack per the entries list | ||
# in config/webpack/shared.js. By default, this list is auto-generated to match everything in | ||
# app/javascript/packs/*.js. In production mode, the digested reference is automatically looked up. | ||
|
@@ -25,8 +29,7 @@ def asset_pack_path(name, **options) | |
# <%= javascript_pack_tag 'calendar', 'data-turbolinks-track': 'reload' %> # => | ||
# <script src="/packs/calendar-1016838bab065ae1e314.js" data-turbolinks-track="reload"></script> | ||
def javascript_pack_tag(*names, **options) | ||
sources = names.map { |name| Webpacker::Manifest.lookup("#{name}#{compute_asset_extname(name, type: :javascript)}") } | ||
javascript_include_tag(*sources, **options) | ||
javascript_include_tag(*asset_source(names, :javascript), **options) | ||
end | ||
|
||
# Creates a link tag that references the named pack file, as compiled by Webpack per the entries list | ||
|
@@ -42,8 +45,23 @@ def javascript_pack_tag(*names, **options) | |
# # In production mode: | ||
# <%= stylesheet_pack_tag 'calendar', 'data-turbolinks-track': 'reload' %> # => | ||
# <link rel="stylesheet" media="screen" href="/packs/calendar-1016838bab065ae1e122.css" data-turbolinks-track="reload" /> | ||
# # In development mode with hot-reloading | ||
# <%= stylesheet_pack_tag('main') %> <% # Default is false for enabled_when_hot_loading%> | ||
# # No output | ||
# | ||
def stylesheet_pack_tag(*names, **options) | ||
sources = names.map { |name| Webpacker::Manifest.lookup("#{name}#{compute_asset_extname(name, type: :stylesheet)}") } | ||
stylesheet_link_tag(*sources, **options) | ||
if Webpacker::DevServer.hot_reloading? | ||
"" | ||
else | ||
stylesheet_link_tag(*asset_source(names, :stylesheet), **options) | ||
end | ||
end | ||
|
||
private | ||
def asset_source(names, type) | ||
names.map do |name| | ||
name_ensuring_ext = "#{name}#{compute_asset_extname(name, type: type)}" | ||
Webpacker::Manifest.asset_source(name_ensuring_ext) | ||
end | ||
end | ||
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.
Why do we need both a configuration option and a ENV value for this again?
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.
Hi @dhh. The issue I'm trying to solve is:
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, all the more reason to separate the two. People who want the live reloading stuff runs the dev server. People who don't want that, don't. Then we can drop these extra configuration bits.