-
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 15 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 |
---|---|---|
|
@@ -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: { | ||
|
@@ -38,10 +37,7 @@ module.exports = { | |
plugins: [ | ||
new webpack.EnvironmentPlugin(JSON.parse(JSON.stringify(env))), | ||
new ExtractTextPlugin(env.NODE_ENV === 'production' ? '[name]-[contenthash].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 | ||
}) | ||
new ManifestPlugin({ writeToFileEmit: true }) | ||
], | ||
|
||
resolve: { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ development: | |
host: localhost | ||
port: 8080 | ||
https: false | ||
hmr: 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. @gauravtiwari @dhh FWIW, the docs for 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 Also, considering usages 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. @ytbryan I agree in the sense that |
||
|
||
test: | ||
<<: *default | ||
|
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 | ||
|
@@ -57,11 +61,22 @@ def data | |
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) | ||
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,54 @@ | ||
# 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 commentThe 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 commentThe 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. |
||
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 hmr? | ||
if enabled? | ||
case ENV["WEBPACKER_HMR"] | ||
when /true/i then true | ||
when /false/i then false | ||
else fetch(:hmr) | ||
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 | ||
|
||
# Uses the hot_reloading_host if appropriate | ||
def base_url | ||
"#{protocol}://#{host}:#{port}" | ||
end | ||
|
||
def fetch(key) | ||
if enabled? | ||
data[:dev_server][key] || defaults[:dev_server][key] | ||
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 | ||
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. Nix this. |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,13 +4,29 @@ 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) | ||
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 | ||
|
||
protected | ||
def ensure_loaded_instance(klass) | ||
raise Webpacker::FileLoader::FileLoaderError.new("#{klass.name}#load must be called first") unless instance | ||
|
@@ -20,6 +36,7 @@ def ensure_loaded_instance(klass) | |
private | ||
def initialize(path) | ||
@path = path | ||
@mtime = File.exist?(path) ? File.mtime(path) : nil | ||
@data = load | ||
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. | ||
|
@@ -41,13 +45,21 @@ 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) | ||
stylesheet_link_tag(*sources_from_pack_manifest(names, type: :stylesheet), **options) | ||
if Webpacker::DevServer.hmr? | ||
"" | ||
else | ||
stylesheet_link_tag(*sources_from_pack_manifest(names, type: :stylesheet), **options) | ||
end | ||
end | ||
|
||
private | ||
def sources_from_pack_manifest(names, type:) | ||
names.map { |name| Webpacker::Manifest.lookup(pack_name_with_extension(name, type: type)) } | ||
names.map { |name| Webpacker::Manifest.asset_source(pack_name_with_extension(name, type: type)) } | ||
end | ||
|
||
def pack_name_with_extension(name, type:) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,31 +17,86 @@ def file_path | |
Webpacker::Configuration.manifest_path | ||
end | ||
|
||
def lookup(name) | ||
# Converts the "name" (aka the pack or bundle name) to to the full path for use in a browser. | ||
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @gauravtiwari are you sure that the 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 - yes that should work 👍 - webpack will reference sub-directories |
||
end | ||
|
||
# Converts the "name" (aka the pack or bundle name) to the possibly hashed name per a manifest. | ||
# | ||
# If Configuration.compile? then compilation is invoked the file is missing. | ||
# | ||
# Options | ||
# throw_if_missing: default is true. If false, then nill is returned if the file is missing. | ||
def lookup(name, throw_if_missing: true) | ||
instance.confirm_manifest_exists | ||
|
||
if Webpacker::Configuration.compile? | ||
compile_and_find!(name) | ||
compile_and_find!(name, throw_if_missing: throw_if_missing) | ||
else | ||
find!(name) | ||
# 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 commentThe 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 commentThe 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 commentThe 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 (More locally-relevant, this should likely use the file watcher to avoid the stat [where possible] even in development.) |
||
raise Webpacker::FileLoader::FileLoaderError.new("Webpacker::Manifest.load must be called first") unless instance | ||
|
||
return find!(name, throw_if_missing: throw_if_missing) | ||
end | ||
end | ||
|
||
# Why does this method exist? Testing? It's not in the README | ||
def lookup_path(name) | ||
Rails.root.join(File.join(Webpacker::Configuration.public_path, lookup(name))) | ||
Rails.root.join(File.join(Webpacker::Configuration.output_path, lookup(name))) | ||
end | ||
|
||
private | ||
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. Indent under private. 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 |
||
def find!(name) | ||
def missing_file_from_manifest_error(bundle_name) | ||
msg = <<-MSG | ||
Webpacker can't find #{bundle_name} in your manifest at #{file_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: | ||
#{instance.data.to_json} | ||
MSG | ||
raise(Webpacker::FileLoader::NotFoundError.new(msg)) | ||
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 is great 👍 🍰 |
||
end | ||
|
||
def missing_manifest_file_error(path_object) | ||
msg = <<-MSG | ||
Webpacker can't find the manifest file: #{path_object} | ||
Possible causes: | ||
1. You have not invoked webpack. | ||
2. You have misconfigured Webpacker's config/webpacker_.yml file. | ||
3. Your Webpack configuration is not creating a manifest. | ||
MSG | ||
raise(Webpacker::FileLoader::NotFoundError.new(msg)) | ||
end | ||
|
||
def find!(name, throw_if_missing: true) | ||
ensure_loaded_instance(self) | ||
instance.data[name.to_s] || | ||
raise(Webpacker::FileLoader::NotFoundError.new("Can't find #{name} in #{file_path}. Is webpack still compiling?")) | ||
value = instance.data[name.to_s] | ||
return value if value.present? | ||
|
||
if throw_if_missing | ||
missing_file_from_manifest_error(name) | ||
end | ||
end | ||
|
||
def compile_and_find!(name) | ||
def compile_and_find!(name, throw_if_missing: true) | ||
Webpacker.logger.tagged("Webpacker") { Webpacker.compile } | ||
find!(name) | ||
find!(name, throw_if_missing: throw_if_missing) | ||
end | ||
end | ||
|
||
def confirm_manifest_exists | ||
raise missing_manifest_file_error(@path) unless File.exist?(@path) | ||
end | ||
|
||
private | ||
def load | ||
return super 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.
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.