Proposal to merge in changes from Webpacker Lite.

[justin808]

Following up from #453 and my discussion with @gauravtiwari, and my article Webpacker Lite: Why Fork Webpacker? and my fork Webpacker Lite.

I'd like to get some consensus on merging in the differences of Webpacker Lite before doing any work on a PR.

YML file and LIVE

Webpacker, from installer file:

development:
   public_output_path: packs
   dev_server:
     host: 0.0.0.0
     port: 8080
     https: false

Webpacker Lite, from README:

development:
  <<: *default
  # generated files for development, in /public/webpack/development
  webpack_public_output_dir: webpack/development
  
  # Default is localhost:3500. You can specify the protocol if needed. Defaults to http://.
  hot_reloading_host: localhost:3500
  
  # Developer note: considering removing this option so it can ONLY be turned by using an ENV value.
  # Default is false, ENV 'HOT_RELOADING' will always override 
  hot_reloading_enabled_by_default: false

1. Should we use host vs individual keys and nested (note, host should have been hostname in webpacker).
2. We need an option regarding whether or not using the webpack-dev-server should be the default, so this can be overridden with an env value, like HOT_RELOADING
3. I'm fine with public_output_path rather than webpack_public_output_dir so long as this is the full path within the public directory where the generated files go, minus the /public part.

Helper differences

1. Putting minimal in the manifest.json, not putting in the port, host, etc. and putting this logic in the Ruby helper. This minimizes the requirement on a custom Webpack config to the bare minimum mapping of bundle names to file names.
2. stylesheet_pack_tag uses the hot reload (webpack-dev-server) to decide if CSS should be included because you can't hot reload extracted CSS.
3. Using the mtime in the FileLoader to account for the chance of using hash in file names for static files, both dev and test env, and the need to get the latest files.

[matthewd]

Should we use host vs individual keys and nested

I like the single string. I'd suggest calling it .._url, though.

We need an option regarding whether or not using the webpack-dev-server should be the default, so this can be overridden with an env value

We're generally not keen on using ENV as a default/primary configuration mechanism. Beyond that note, some way to configure & override it sounds useful, but I don't have enough context to have an opinion on how it should look.

[justin808]

@matthewd wrote:

We're generally not keen on using ENV as a default/primary configuration mechanism. Beyond that note, some way to configure & override it sounds useful, but I don't have enough context to have an opinion on how it should look.

The ENV part is totally optional...Allows having different procfiles for using static assets vs. hot loading.

And regarding the URL vs. host, host is technically the right word, as the URL includes the full path. I think the MDN URL docs are pretty "official".

@gauravtiwari We've got one vote for the simpler, non-nested, 4 key approach.

[gauravtiwari]

@justin808 Awesome. Lets make this work 👍 🍰

YML config

The reasons why the dev server config is nested and broken into different keys are:

1. Matches the options for dev server config so it's easy to reason - https://webpack.js.org/configuration/dev-server/
2. Simple to parse - no regex tricks. These values are used inside ruby binstubs too.

Helpers

1. Manifest.json: The reason why the full paths are there in manifest.json is because we don't have to duplicate logic on ruby side and instead just perform a simple lookup - https://github.com/rails/webpacker/blob/master/lib/webpacker/manifest.rb#L16 vs constructing base url - https://github.com/shakacode/webpacker_lite/blob/master/lib/webpacker_lite/configuration.rb#L23

I agree it's trivial to do but we wanted to keep things simple. Now, it's responsibility of Webpack and plugins to return fully qualified asset paths based on environment instead of mixing responsibilities and logic.

Happy to hear more thoughts on this :)

2. Hot reloading: What do you think if we just have a hot option inside yml file and use that instead of ENV? Again as pointed earlier - for easy parsing and match with options dev server already provides - https://webpack.js.org/configuration/dev-server/#devserver-hot

development:
   public_output_path: packs
   dev_server:
     host: 0.0.0.0
     port: 8080
     hot: true
     https: false

3. Recompilation: Yepp lets do that 👍 We already started discussing this here Lazy compile in test env #360 (comment). Basically, this is what I did in an earlier PR -

    def checksum
      files = Dir["#{Webpacker::Configuration. source_path}/**/*", "package.json", "yarn.lock"].reject { |f| File.directory?(f) }
      files.map { |f| File.mtime(f).utc.to_i }.max.to_s
    end
  
    Rails.cache.fetch(["webpacker", "manifest", checksum]) do
      # .... compile code
    end

What do you think?

[justin808]

@gauravtiwari, in order for us to agree on the result, we have to first agree on goals.

Common Goals

React on Rails needs these 2 things:

1. Minimal requirements put on the webpack config:

• Creation of a manifest.json.
• Get common Rails/Webpack configuration, like the output directory, from webpackConfigLoader (see below).

2. stylesheet_pack_tag supports hot reloading (per prior messages).

Migration for React on Rails Apps

In order to make any migration for existing React on Rails users to use Webpacker from Webpacker Lite, I'd suggest the following are mandatory, along with the migration steps for React on Rails apps:

1. The YAML file for configuration is SIMPLE.

• Change the configuration file name from webpacker_lite.yml to webpacker.yml.
• Inside the YAML config file:
  • You may remove the manifest key as the filename of the created manifest must be manifest.json
  • Change the key webpack_public_output_dir to public_output_path.

2. Change the Gemfile to point to 'webpacker' from 'webpacker_lite'.
3. Change the loading of the 'webpackConfigLoader' in your webpack configuration:

• From: const webpackConfigLoader = require('react-on-rails/webpackConfigLoader');
• To: const webpackConfigLoader = require('webpacker/webpackConfigLoader');

References from React on Rails

1. webpackConfigLoader from React on Rails
2. Sample Webpack config from React on Rails generator.

Action Plan

If we can agree on these, my action plan would be:

1. Create a PR with needed changes for Webpacker, along with pushing a version beta of the gem and NPM package (maybe using some temp name).
2. Create a PR for React on Rails that

• Uses those the beta versions.
• Has migration instructions and tests!

[gauravtiwari]

@justin808 Sounds good 👍 with one exception i.e. webpack config. You see, webpacker is all about webpack and therefore, we can't do much there - the idea is to keep it simple and production ready much like Sprockets.

As discussed earlier, the best route is - you ship your own version of webpack config like you have been doing so far for react_on_railsand just use webpacker helpers for serving assets. It will use manifest.json, which will be written by your version of webpack config so I think everything will work as usual.

We can merge in hot reloading changes but without using environment and instead use dedicated config option inside dev_server like explained earlier.

Happy to help out with these changes 👍 😄 Doesn't look like a lot though

[justin808]

@gauravtiwari Your suggestions are not clear to me. I'd like to have React on Rails depend on Webpack so that:

1. The YAML configuration is ultra simple and clean.
2. Webpacker requires the webpack setup to only create a minimal manifest.json with no URLS.

Please see my message above again and comment on it line by line.

[justin808]

@kaspth @gauravtiwari @dhh Any feedback on this? I'm giving a number of talks around the US on React on Rails and everybody is interested in how Webpacker relates to React on Rails.

I'd prefer if React on Rails could depend on Webpacker so long as the simple part that React on Rails needs could remain stable.

[gauravtiwari]

@justin808 Lets go over this together step by step.

Goal

• Make react_on_rails depend on webpacker instead of webpacker_lite

React on Rails current setup

• Ships with it's own webpack config - https://github.com/shakacode/react_on_rails/blob/master/lib/generators/react_on_rails/templates/base/base/client/webpack.config.js
• Uses extracted view and configuration helpers from webpacker called webpacker_lite with two additions - hot reloading and how asset paths are saved in manifest.json - https://github.com/shakacode/react_on_rails/blob/master/lib/generators/react_on_rails/templates/base/base/config/webpacker_lite.yml

Minimal steps to integrate with Webpacker

Since react_on_rails has it's own defaults and opinions in regards to webpack setup it would make sense to not bring that into discussion at all and instead figure out how react_on_rails can depend on webpacker view helpers as it is now.

I feel the most straightforward path is:

1. (React on Rails) Ship your own version of webpack config and yml file during installation of react_on_rails i.e. - webpack.config.js etc. and webpacker.yml

2. (React on Rails) Update webpacker.yml that ships with react_on_rails to use keys similar to what webpacker provides (at least the core config). For ex - paths and dev_server for hot reloading

default: &default
  source_path: app/javascript
  source_entry_path: packs
  public_output_path: packs

development:
   # https://github.com/shakacode/react_on_rails/blob/master/lib/generators/react_on_rails/templates/base/base/config/webpacker_lite.yml#L17
   # Instead of - hot_reloading_enabled_by_default
   dev_server:
     hot: true

Just make sure the core settings are there for webpacker, which are paths and dev_server hot option

3. (Webpacker) would need to add this one change for hot reloading i.e. -

  def stylesheet_pack_tag(name, **options)
    return nil if Webpacker::DevServer.hot?
    stylesheet_link_tag(Webpacker::Manifest.lookup("#{name}#{compute_asset_extname(name, type: :stylesheet)}"), **options)
  end

4. The last requirement is how urls are stored inside manifest.json.

Webpacker considers manifest.json as a single source for assets - meaning it assumes that whenever a lookup will be performed manifest.json will return full asset path (absolute or relative) depending on env - https://github.com/rails/webpacker/blob/master/lib/install/config/webpack/configuration.js#L27

React on Rails on the other hand appends base_url after lookup - https://github.com/shakacode/webpacker_lite/blob/master/lib/webpacker_lite/helper.rb#L62

I am not sure what cons are to approach Webpacker is using? Feel free to point out.

---

In summary following changes are required to make the migration possible:

React on Rails

1. Use same webpacker.yml as Webpacker (only core config needs to be same)

default: &default
  source_path: app/javascript
  source_entry_path: packs
  public_output_path: packs

development:
   # instead of - hot_reloading_enabled_by_default
   dev_server:
     hot: true

2. Update manifest.json to include full publicPath.

Webpacker

1. Add hot reloading option so it returns nil for stylesheets

---

Does this makes sense?

[justin808]

@gauravtiwari

1. The YAML file requires allowing a simpler config. Very many projects will not use "packs" as different files are needed for server vs. client rendering with React. As far as I can tell, the ruby part that React on Rails needs does not use things like source_path, etc. So if these are missing, the Ruby helpers will work fine.
2. React on Rails users need to allow the default for using the webpack-dev-server to be off (config value) and to allow overriding this via an ENV value. See Procfile.hot vs. [Procfile.static](https://github.com/shakacode/react_on_rails/blob/master/spec/dummy/Procfile.static
3. It's simply impossible for me to require React on Rails users to put in the URLs in the manifest.json. I got a ton of pushback when we started doing that.

@gauravtiwari please read my discussion above again. My original position stands on this.

[gauravtiwari]

@justin808 Sure

1. No problem, just use whatever you need and change as you like 👍 I think as long as you ship your own yml and webpack config files I don't see any problem.

2. I understand but in Webpacker dev-server is used as default in development env, even without hot reloading. As described in previous comments yml is using separate keys to match dev-server official configuration so it makes sense to a user when adding a new one or simple changing it.

3. What were the reasons for push back? If you could share here and it makes sense we can surely change it in Webpacker too 👍

[justin808]

@gauravtiwari:

1. No problem, just use whatever you need and change as you like 👍 I think as long as you ship your own yml and webpack config files I don't see any problem.

Good! the Ruby code can even support both keys. Your JS code only needs the "packs" style keys.

2. I understand but in Webpacker dev-server is used as default in development env, even without hot reloading. As described in previous comments yml is using separate keys to match dev-server official configuration so it makes sense to a user when adding a new one or simple changing it.

1. We've never found any compelling reason to use the webpack-dev-server without hot reloading enabled. In those cases, it's nice to switch back and forth. For example, sometimes one does not want hot reloading changing the browser when one is also tweaking the browser to match up some artwork. An accidental file being saved would result in all changes being lost in the browser.
2. Using statically compiled assets in development matches closer to production.

3. What were the reasons for push back? If you could share here and it makes sense we can surely change it in Webpacker too 👍

The current Rails manifest does not put in URLs. The default for Webpack does not do this. Semantically, the manifest is a mapping of basic names to hashed names. Why should the manifest care what the web server is doing. The comment I got over and over again was something along the lines of "this makes no sense to put URLs in the manifest". Note, React on Rails does not require any sort of webpack config. While we generate a super simple example, that is just an example.

More importantly, why should React on Rails, as a framework, put additional, unnecessary requirements on the usage?

Both Webpacker and React on Rails should place the absolute minimum requirements on our users. To that ends, just use one field for the "host" in the YAML file ("host" is semantically wrong currently according to the MDN docs for URL). Our libraries should do the extra work of parsing the host (it's easy.

Before we can move forward, we all need to agree on the last point:
Both Webpacker and React on Rails should place the absolute minimum requirements on our users.

[gauravtiwari]

@justin808 Great 👍

1. In regards to dev server config naming - all keys taken from here https://webpack.js.org/configuration/dev-server/#devserver-host-cli-only for clarity. Webpack dev server is better tool for development env - it provides live reloading by default and serves from memory so usually faster than regular webpack watcher.

2. For manifest, I see your point and it's mainly related to aesthetics of how one wants to keep it. Webpack by default ships with publicPath option - https://webpack.js.org/configuration/output/#output-publicpath - and all we are doing is using that option and then the compiler outputs the path to manifest. There is nothing special about this setup.

Basically, there are no additional requirements for user - it's all standard webpack.

[justin808]

@gauravtiwari

In regards to dev server config naming - all keys taken from here https://webpack.js.org/configuration/dev-server/#devserver-host-cli-only for clarity.

While that's reasonable, I'd argue that MDN is more official. Are we strongly objecting to simplifying to alternately supporting a non-nested host which can optionally include https and a port?

Webpack dev server is better tool for development env - it provides live reloading by default and serves from memory so usually faster than regular webpack watcher.

Do you have data on any large projects? My company ShakaCode has worked on many large projects and we have not found this to be the case. In fact, we NEED to use the Webpack DLL plugin for development. This may still be possible.

For manifest, I see your point and it's mainly related to aesthetics of how one wants to keep it. Webpack by default ships with publicPath option - https://webpack.js.org/configuration/output/#output-publicpath - and all we are doing is using that option and then the compiler outputs the path to manifest. There is nothing special about this setup.

1. By default, this is blank.
2. It seems this would require a tighter binding between deployment and compilation, so this topic seems to be a non-starter for the React on Rails folks. Plus, why not have the Ruby code know?

Basically, there are no additional requirements for user - it's all standard webpack.
Do you still believe this to be the case per my discussion above?

I'd recommend that (per my note above):

1. The library (webpacker) could allow either config setup, to achieve the aim of max simplicity and so as to not require migration from either set of webpacker or react_on_rails users.
2. I see no reason to not allow an ENV value to allow overriding the webpack-dev-server setting. ENV's should always be able to override config files.
3. Drop the placement of any deployment information from a simple manifest file. webpacker_lite already has the code to have in the Ruby setup.

[nickpith]

While that's reasonable, I'd argue that MDN is more official. Are we strongly objecting to simplifying to alternately supporting a non-nested host which can optionally include https and a port?

@justin808 - I don't see the desire to change the name and change the config file as you suggest. I agree with @gauravtiwari here. Yes, it would make it easier for you to write migration instructions for React on Rails consumers to upgrade but I think you can easily build that into a script/Rake task as well and consumers will not know the difference. Once it is set, this will never change for consumer so why force a large change like this on other consumers of Webpacker?

To me, the perceived benefit does not outweigh the cost of making a breaking change for something this trivial.

[nickpith]

I do agree with having an option for the manifest to contain relative paths and not full URLs.

Also, allowing ENV variables to override certain configs that change across different deployment environments is good especially for Docker deployments where you want to generate one image but flex config without having to rebuild the container.

[nickpith]

On more thought with regard to the last comment about ENV variables, I think you can reference the ENV variables in the YML files if the system parsing the files passes the file thru ERB. I would have to check Webpack source but it seems like it should be doing it that way. Maybe that is a good compromise?

[dhh]

Yes. We use ENV references in database.yml as well. Let's stick with that.

…

On Jun 14, 2017, at 02:40, Nick Smith ***@***.***> wrote: On more thought with regard to the last comment about ENV variables, I think you can reference the ENV variables in the YML files if the system parsing the files passes the file thru ERB. I would have to check Webpack source but it seems like it should be doing it that way. Maybe that is a good compromise? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[justin808]

@dhh Unfortunately, we can't use ENV references in the YAML via ERB, as that involves the Ruby interpolation and we need to read the YAML file from JavaScript. @robwise and I went down this path. If anybody can show otherwise, that would be superb. Here's the YAML spec.

[gauravtiwari]

@justin808

Do you have data on any large projects? My company ShakaCode has worked on many large projects and we have not found this to be the case. In fact, we NEED to use the Webpack DLL plugin for development. This may still be possible.

Here is the official recommendation - https://webpack.js.org/guides/development/#choosing-a-tool

1. The library (webpacker) could allow either config setup, to achieve the aim of max simplicity and so as to not require migration from either set of webpacker or react_on_rails users.

If react_on_rails doesn't use any webpack internals this shouldn't be a problem - just use view helpers. I hope you understand that use cases for both gems are different so compatibility won't be 100% plus there are some conflicting concerns in regards to webpack setup.

2. I see no reason to not allow an ENV value to allow overriding the webpack-dev-server setting. ENV's should always be able to override config files.

For webpacker, there is no need because one can pass CLI options and override at run-time. Apart from that all other settings are configurable per environment (just paths).

./bin/webpack-dev-server --host example.com --port 8081 --inline true --hot false

Anything else I am missing?

3. Drop the placement of any deployment information from a simple manifest file. webpacker_lite already has the code to have in the Ruby setup.

Happy to drop it but from our discussion I haven't found any good reason to do so. In the end it's just manifest, which contains meta data for CDN/Server and asset paths. The setup is simple, implicit and just works.

[gauravtiwari]

Alright just saw this issue - reactjs/react-rails#739 and sounds like a good reason to remove CDN/host information from manifest.json.

[gauravtiwari]

BTW, react-rails provides server rendering with webpacker. Why can't react_on_rails?

[robwise]

ReactOnRails can provide it the same way react-rails does with webpacker, but webpacker forces you to use a single bundle across the entire app for both server and client rendering, which is very limiting.

[justin808]

@gauravtiwari:

1. I agree on being OK for webpack-dev-server to be "default" for Rails.env == "development".
2. However, we still need env specific config values and the ability to override because:

• Sometimes you might have a slightly different locally named "development" environment.
• You might want to be sure your static compilation is working fine.

For webpacker, there is no need because one can pass CLI options and override at run-time. Apart from that all other settings are configurable per environment (just paths).
./bin/webpack-dev-server --host example.com --port 8081 --inline true --hot false

Anything else I am missing?

Yes. Under no circumstances do we want to require using a Ruby wrapper around the JavaScript commands. Nobody else in the JS community is going to do this.

3. Are you now in agreement on dropping this and going with the webpacker_lite way of a simple manifest.json?

Drop the placement of any deployment information from a simple manifest file. webpacker_lite already has the code to have in the Ruby setup.
Happy to drop it but from our discussion I haven't found any good reason to do so. In the end it's just manifest, which contains meta data for CDN/Server and asset paths. The setup is simple, implicit and just works.

@gauravtiwari Where do we stand with what we agree and disagree upon?

[gauravtiwari]

@robwise By default webpacker supports multiple bundles or packs. So one pack could be client rendered and other could be server rendered (isolated i.e. no reference to windows etc.).

packs
  - app-client.js
  - app-server.js

outputs two bundles/packs

<%= react_component 'app-client' %>
<%= react_component 'app-server', { prerender: true } %>

[gauravtiwari]

@justin808

Are you now in agreement on dropping this and going with the webpacker_lite way of a simple manifest.json?

Yes, makes sense to not include host information per reactjs/react-rails#739 👍

2. Dev Server - Great 👍

Yes. Under no circumstances do we want to require using a Ruby wrapper around the JavaScript commands. Nobody else in the JS community is going to do this.

The idea behind ruby wrapper for webpack and dev-server executables is to bootstrap correct settings per environment so it's simple and implicit. I understand JS community wouldn't have it because a JS app would be setup differently compared to a Rails app. The binstubs are commonly used within a Rails app

# This is env aware and applies all necessary settings for executables
#  instead of of passing env values through package.json and then doing the same thing
./bin/webpack-dev-server

[justin808]

@gauravtiwari This sounds great. Please comment on whether we should have an option or different helper, per this issue: shakacode/webpacker_lite#20 (comment)

Here is the readme change, from @robwise

https://github.com/shakacode/webpacker_lite/pull/22/files

Unmanifested Files

Some special situations may call for including files output by webpack that are not included in the
manifest. In this case, these assets must not be hashed, as without the manifest, Rails has no
manifest file, there is no way to translate an unhashed filename to a hashed one. An example use case
might be when using a separate webpack config to create a vendor DLL bundle using the Webpack DLL
Plugin. Because this is only used in development, it is not necessary to hash, and because it is made
in a different webpack config, it cannot share the same manifest file.

<%# app/views/layouts/application.html.erb %>
<%= unmanifested_javascript_pack_tag('vendor-dll') %>
<%= unmanifested_stylesheet_pack_tag('vendor-dll') %>

[justin808]

@gauravtiwari I'm starting this one. @robwise and I need to know what to do about the unmanifested files.

[p0wl]

Regarding your dll example, it is possible to use the same file for different webpack builds when using the manifest plugin with the cache option:

const manifest = require(join(resolve(paths.output, paths.entry), 'manifest.json'))
...
new ManifestPlugin({ fileName: 'manifest.json', publicPath, writeToFileEmit: true, cache: manifest }),

This way, the plugin will amend lines.

If this is the only example for unmanifested files, I think it would be better to not include such functionality, because you also mentioned that it was only for development (btw: I suggest to use the dll plugin in production too, because it is very efficient for long term caching)

[robwise]

@p0wl You're a genius!!! That manifest caching snippet totally worked! Cheers!

We still use a vendor bundle in production, but we switch to doing it via the common chunk plugin instead. At the time we set up our architecture it was still being recommended to only use DLL in development as it was not production-ready. I'm not sure if that's still the case or not.

[gauravtiwari]

@justin808 Lets do what we have discussed earlier first and we can look into dll helper later.
@p0wl @robwise But I guess that would still require separate webpack config for DLL setup right?

[p0wl]

Yes, DLL needs an additional webpack config. But if @justin808's proposal of "Unmanifested Files" is only needed when using the DLL plugin, I would recommend against it, because I think it is not a good idea to create another possibility for rails to access webpacker files (right now it is ONLY the manifest.json, which is pretty awesome I think).

[gauravtiwari]

Thanks 👍 Yeah, that makes sense - could be confusing for most users and probably redundant.

[robwise]

Yeah, I closed that PR since I was able to do it with the caching thing. It does require two configs, yes. We also use a third config for our server rendering bundle as well, although that's for convenience, it technically doesn't have to be done that way.

[gauravtiwari]

@justin808 Hey - Any news on this?

[justin808]

@gauravtiwari I'm working on this today. I need to do something about #571, skipping the asset host for server rendering. Suggestions?

[gauravtiwari]

@justin808 Can we use this instead? https://github.com/rails/webpacker/blob/master/lib/webpacker/manifest.rb#L24

I think this will return full local path for a given asset.

[justin808]

@gauravtiwari I think I found a simpler solution (from my original one)

shakacode/webpacker_lite#23

Please advise if this small API addition is OK for rails/webpacker.

Or maybe we don't need in the view helper? and we can call the manifest method? I'll save that for the merge. Please advise.

[gauravtiwari]

@justin808 Since we are going to use this internally we can use lookup_path for now. Can you think of any other use cases for pack_path view helper? If not, lets leave that for now :)

[justin808]

I'll need to access lookup_path from React on Rails. I'm guessing this should be feasible. However, we need to document this as part of the public API as this affects any other code that is server rendering.

[gauravtiwari]

@justin808 Yepp lets document it. I think react-rails is already using this as public API for server rendering.

[justin808]

@gauravtiwari we should organize the files so that anything exposed publicly is more obvious. Otherwise, some "helpful" refactoring will break downstream libraries. Maybe we should have a wrapper class of these public APIs so make the public surface area clear.

[gauravtiwari]

We have already merged/added changes discussed here. @justin808 Please open a new issue with remaining issues (if any?) and lets discuss it there.