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

Support React on Rails by merging Webpacker Lite #601

Closed
Show file tree
Hide file tree
Changes from 3 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
10 changes: 10 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,13 @@ gem "rubocop", ">= 0.47", require: false
group :test do
gem "minitest", "~> 5.0"
end

if ENV["USE_PRY"]
Copy link
Contributor Author

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.

Copy link
Contributor Author

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

gem "awesome_print"
gem "pry"
gem "pry-byebug"
gem "pry-doc"
gem "pry-rails"
gem "pry-rescue"
gem "pry-stack_explorer"
end
Copy link
Member

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.

36 changes: 35 additions & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,19 @@ GEM
tzinfo (~> 1.1)
arel (7.1.4)
ast (2.3.0)
awesome_print (1.8.0)
binding_of_caller (0.7.2)
debug_inspector (>= 0.0.1)
builder (3.2.3)
byebug (9.0.6)
coderay (1.1.1)
concurrent-ruby (1.0.5)
debug_inspector (0.0.3)
erubis (2.7.0)
globalid (0.3.7)
activesupport (>= 4.1.0)
i18n (0.8.1)
interception (0.5)
loofah (2.0.3)
nokogiri (>= 1.5.9)
mail (2.6.4)
Expand All @@ -71,6 +78,24 @@ GEM
parser (2.4.0.0)
ast (~> 2.2)
powerpack (0.1.1)
pry (0.10.4)
coderay (~> 1.1.0)
method_source (~> 0.8.1)
slop (~> 3.4)
pry-byebug (3.4.2)
byebug (~> 9.0)
pry (~> 0.10)
pry-doc (0.10.0)
pry (~> 0.9)
yard (~> 0.9)
pry-rails (0.3.6)
pry (>= 0.10.4)
pry-rescue (1.4.5)
interception (>= 0.5)
pry
pry-stack_explorer (0.4.9.2)
binding_of_caller (>= 0.7)
pry (>= 0.9.11)
Copy link
Member

Choose a reason for hiding this comment

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

Think you forgot to run bundle after removing these gems ;)

rack (2.0.1)
rack-test (0.6.3)
rack (>= 1.0)
Expand Down Expand Up @@ -106,6 +131,7 @@ GEM
ruby-progressbar (~> 1.7)
unicode-display_width (~> 1.0, >= 1.0.1)
ruby-progressbar (1.8.1)
slop (3.6.0)
sprockets (3.7.1)
concurrent-ruby (~> 1.0)
rack (> 1, < 3)
Expand All @@ -121,17 +147,25 @@ GEM
websocket-driver (0.6.5)
websocket-extensions (>= 0.1.0)
websocket-extensions (0.1.2)
yard (0.9.9)

PLATFORMS
ruby

DEPENDENCIES
awesome_print
bundler (~> 1.12)
minitest (~> 5.0)
pry
pry-byebug
pry-doc
pry-rails
pry-rescue
pry-stack_explorer
rails
rake (>= 11.1)
rubocop (>= 0.47)
webpacker!

BUNDLED WITH
1.14.6
1.15.3
26 changes: 24 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,14 @@ development:
host: 0.0.0.0
port: 8080
https: false
hot: false
```

If you have hot 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
Expand Down Expand Up @@ -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` 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.
Copy link
Member

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?

Copy link
Contributor Author

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:

  • We can't put Ruby in the YML file, since the YML file is read by JavaScript.
  • On bigger teams, it's common to have some developers wishing to do hot reloading or the webpack-dev-server, while others prefer just to have the files built in a similar manner to production.
  • We don't want some developers on a team to have the webpack.yml file modified just for their own environment.
  • We want a team to have different Procfiles depending if a developer wants to use these options for hot-reloading and the webpack-dev-server.

Copy link
Member

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.


### 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

Expand Down Expand Up @@ -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.
Copy link
Member

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).


Other alternatives include [react-rails](https://github.com/reactjs/react-rails) and
[webpacker-react](https://github.com/renchap/webpacker-react) for more advanced react integration.

If you're not concerned with view helpers to pass props or server rendering, can do it yourself:

```erb
<%# views/layouts/application.html.erb %>
Expand Down Expand Up @@ -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.

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


Here is a sample system test case with hello_react example component:

Expand Down
4 changes: 1 addition & 3 deletions lib/install/config/webpack/shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ module.exports = {

output: {
filename: '[name].js',
path: output.path,
publicPath: output.publicPath
Copy link
Contributor Author

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.

path: output.path
},

module: {
Expand All @@ -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,
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 Can you help me with the webpack changes? I didn't see any docs on how to test this.

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 I created a test repo and tested the basic generator. What else?

Copy link
Member

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 👍

writeToFileEmit: true
})
],
Expand Down
1 change: 1 addition & 0 deletions lib/install/config/webpacker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ development:
host: localhost
port: 8080
https: false
hot: false

test:
<<: *default
Expand Down
10 changes: 6 additions & 4 deletions lib/webpacker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 :) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I meant, change from load to load_instance. I am happy to see it too - just a lot of unrelated change to review in one PR :)

Copy link
Member

Choose a reason for hiding this comment

The 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.

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 and @gauravtiwari if you want me to rename all the load_instance and load_data to load, I'll do that. I'll wait for your confirmation on this one.

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand All @@ -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)
30 changes: 25 additions & 5 deletions lib/webpacker/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,21 @@

class Webpacker::Configuration < Webpacker::FileLoader
class << self
def reset
@defaults = nil
super
end
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


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
Expand Down Expand Up @@ -45,19 +54,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.dev_server?
Webpacker::DevServer.base_url
else
# Ensure we start with a slash so that the asset helpers don't prepend the default asset
# pipeline locations.
Copy link
Member

Choose a reason for hiding this comment

The 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.

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 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

Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Contributor Author

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.

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 follow? In which cases are we calling #load to mean different things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HashWithIndifferentAccess.new(YAML.load(File.read(@path))[Webpacker.env])
end
end
91 changes: 91 additions & 0 deletions lib/webpacker/dev_server.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# Same convention as manifest/configuration.rb

# Loads webpacker configuration from config/webpacker.yml

require "webpacker/configuration"

class Webpacker::DevServer < Webpacker::FileLoader
class << self
def dev_server?
Copy link
Member

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.

env_val = env_value("WEBPACKER_DEV_SERVER")

# override dev_server setup WEBPACKER_DEV_SERVER=FALSE
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?
Copy link
Member

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

end

# read settings for dev_server
def hot?
return false unless dev_server?
env_val = env_value("WEBPACKER_HMR")
return env_val unless env_val.nil?
fetch(:hot)
Copy link
Member

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

end

def host
fetch(:host)
end

def port
fetch(:port)
end

def https?
fetch(:https)

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

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.

def env_value(env_key)
if ENV[env_key].present?
val = ENV[env_key]
val_upcase = val.upcase
return true if val_upcase == "TRUE"
return false if val_upcase == "FALSE"
raise new ArgumentError("#{env_key} value is #{val}. Set to TRUE|FALSE")
end
# returns nil
Copy link
Member

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.

end

def dev_server_values
data.fetch(:dev_server, nil)
end
Copy link
Member

Choose a reason for hiding this comment

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

Nix this.


def fetch(key)
return nil unless dev_server?
dev_server_values.fetch(key, dev_server_defaults[key])
Copy link
Member

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

end

def data
load_instance if Webpacker.env.development?
unless instance
raise Webpacker::FileLoader::FileLoaderError.new("Webpacker::DevServer.load_data must be called first")
end
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

instance.data
end

def dev_server_defaults
@defaults ||= Webpacker::Configuration.defaults[:dev_server]
end
Copy link
Member

Choose a reason for hiding this comment

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

Nix this.

end

private
def load_data
Webpacker::Configuration.instance.data
end
end
2 changes: 1 addition & 1 deletion lib/webpacker/env.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def file_path
end

private
def load
def load_data
environments = File.exist?(@path) ? YAML.load(File.read(@path)).keys : [].freeze
return ENV["NODE_ENV"] if environments.include?(ENV["NODE_ENV"])
return Rails.env if environments.include?(Rails.env)
Expand Down
Loading