Looking great! some comments!

Reviewed 18 of 18 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

Procfile.hot, line 6 at r1 (raw file):

# Development rails requires both rails and rails-assets
# (and rails-server-assets if server rendering)
rails: REACT_ON_RAILS_ENV=HOT rails s -b 0.0.0.0

this HOT var value applies to the env helper for the assets, not the new stuff.

This will change:
https://github.com/shakacode/react_on_rails/blob/master/docs/api/ruby-api-hot-reload-view-helpers.md#hot-reloading-view-helpers

app/views/layouts/application.html.erb, line 9 at r1 (raw file):

 hot: 'application_non_webpack',
 media: 'all',
 'data-turbolinks-track' => "reload") %>

missing the stylesheet tag

client/server-rails-hot.js, line 8 at r1 (raw file):

import webpackConfig from './webpack.client.rails.hot.config';
const { devServer: devServerConfig, publicPath } = require('./webpackConfigLoader.js');

why require and not import?

client/webpack.client.base.config.js, line 53 at r1 (raw file):

 output: {
 filename: '[name]-bundle.js',

should this have a hash?

client/webpackConfigLoader.js, line 1 at r1 (raw file):

/* eslint import/no-dynamic-require: 0 */

we can eventually put this into the react-on-rails npm module

config/webpack/development.server.yml, line 15 at r1 (raw file):

production:
 <<: *default

Maybe this yaml inheritance is overkill? We'll really only use the dev server for development.

config/webpack/paths.yml, line 14 at r1 (raw file):

 - .sass
 - .scss
 - .css

Are we using all these settings above?

I'd put some comments in this file indicating what's used and just documentation.

And maybe comment out the lines that are just documentation.

Comments from Reviewable

Review status: 14 of 18 files reviewed at latest revision, 7 unresolved discussions.

Procfile.hot, line 6 at r1 (raw file):

If we use the webpacker helpers, they'll need a similar hot, static options. Since we need the css in static but not in hot loading. If I leave in the stylesheet_pack_ tag, it will throw an error when hot loading.

app/views/layouts/application.html.erb, line 9 at r1 (raw file):

If I leave in the stylesheet_pack_ tag, it will throw an error when hot loading. We need hot and static options

client/server-rails-hot.js, line 8 at r1 (raw file):

We require a lot of other places, but it would be better if it was consistent.

client/webpack.client.base.config.js, line 53 at r1 (raw file):

I'm not sure

config/webpack/development.server.yml, line 15 at r1 (raw file):

Then we might not need the yaml at all.

Comments from Reviewable

Review status: 14 of 18 files reviewed at latest revision, 7 unresolved discussions.

app/views/layouts/application.html.erb, line 9 at r1 (raw file):

or make so the stylesheet helper includes nothing if hot reloading

Comments from Reviewable

Review status: 14 of 18 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

client/webpack.server.rails.build.config.js, line 25 at r2 (raw file):

} catch (ex) {
 console.error(ex);
 console.log('Make sure the client manifest is created');

Let's put in the location where we expect it! better error message!

and indicate where this is configured.

client/webpackConfigLoader.js, line 8 at r2 (raw file):

const configPath = resolve('..', 'config', 'webpack');
const paths = safeLoad(readFileSync(join(configPath, 'paths.yml'), 'utf8'))[env.NODE_ENV];
const devServer = safeLoad(readFileSync(join(configPath, 'development.server.yml'), 'utf8')).development;

this is a lot to read when chained.

let's break up to two lines

and let's think about abstracting this into react-on-rails npm

config/webpack/development.server.yml, line 15 at r1 (raw file):

I guess this is fine for now if this is what webpacker is doing.

config/webpack/paths.yml, line 14 at r1 (raw file):

https://webpack.js.org/configuration/resolve/#resolve-extensions

I disagree on putting this in the paths file.

I think it's confusing to import non JS/JSX files without the extension. In other words, if I'm importing a PNG, I want to see in the code that it's a PNG.

@robwise Agree?

Comments from Reviewable

Looking good! Some comments.

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

Procfile.hot, line 6 at r1 (raw file):

We need to modify the stylesheet pack tag to recognize that we're hot reloading.

app/views/layouts/application.html.erb, line 9 at r1 (raw file):

the latter!

client/server-rails-hot.js, line 8 at r1 (raw file):

OK. Not sure on this one. Leave for now.

@robwise?

client/webpack.client.base.config.js, line 53 at r1 (raw file):

Yes it should!

otherwise browser won't know file changed!

Comments from Reviewable

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

config/webpack/paths.yml, line 14 at r1 (raw file):

I don't get why we're still specifying any webpack settings outside of webpack? This still won't work for builds that require multiple configs

Comments from Reviewable

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

client/webpack.client.base.config.js, line 53 at r1 (raw file):

then this is wrong as well?
https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/webpack.client.rails.build.config.js#L16

config/webpack/paths.yml, line 14 at r1 (raw file):

@justin Ditch the extentions part? Or the whole yaml? Place it into webpackConfigLoader maybe?

Comments from Reviewable

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

client/webpack.client.base.config.js, line 53 at r1 (raw file):

I'm quite sure we need the hash. @robwise Please confirm.

and yes, in all places.

config/webpack/paths.yml, line 14 at r1 (raw file):

The webpackConfigLoader should focus on only bringing in the parts of the yaml file need.

This yaml files should be slimmer.

yes, ditch the extensions and hard code only .js .jsx

Comments from Reviewable

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

config/webpack/paths.yml, line 2 at r2 (raw file):

# Restart webpack-watcher or webpack-dev-server if you make changes here
default: &default

we can clean this up per the new webpacker_lite

config/webpack/paths.yml, line 19 at r2 (raw file):

 # - .svg
 # - .gif
 # - .jpeg

we can remove the commented extensions

Comments from Reviewable

@kaizencodes Let's address the comments and get this ready to merge.

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 15 of 15 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

Procfile.dev, line 10 at r4 (raw file):

# Run the hot reload server for client development
hot-assets: sh -c 'rm -rf public/webpack/* || true && bundle exec rake react_on_rails:locale && HOT_RAILS_PORT=3500 yarn run hot-assets'

this should only delete the dev files, not all files

Procfile.hot, line 9 at r4 (raw file):

# Run the hot reload server for client development
hot-assets: sh -c 'rm -rf public/webpack/* || true && bundle exec rake react_on_rails:locale && HOT_RAILS_PORT=3500 yarn run hot-assets'

see above comment --> only delete dev

Procfile.static, line 5 at r4 (raw file):

# Build client assets, watching for changes.
rails-client-assets: rm -rf public/webpack/* || true && bundle exec rake react_on_rails:locale && yarn run build:dev:client

again

app/views/layouts/application.html.erb, line 6 at r4 (raw file):

 <title>RailsReactTutorial</title>
 <%= stylesheet_pack_tag(static: ['vendor', 'app'],

these should change to -bundle

app/views/layouts/application.html.erb, line 11 at r4 (raw file):

 <%= javascript_pack_tag('vendor', 'data-turbolinks-track': true) %>)
 <%= javascript_pack_tag('app', 'data-turbolinks-track': true) %>)

add -bundle

client/server-rails-hot.js, line 12 at r4 (raw file):

const webpackConfigLoader = require('react-on-rails/webpackConfigLoader');
const configPath = resolve('..', 'config', 'webpack');
const { devServer: devServerConfig, publicPath } = webpackConfigLoader(configPath);

did we lint? extra space?

config/initializers/react_on_rails.rb, line 89 at r4 (raw file):

 # For any asset matching this regex, non-digested symlink will be created
 # To disable symlinks set this parameter to nil.
 config.symlink_non_digested_assets_regex = nil

need to make this clear in the upgrade doc

we should just delete this and the default is nil

config/webpack/paths.yml, line 5 at r4 (raw file):

 assets: webpack
 manifest: manifest.json
 node_modules: client/node_modules

dont' need

Comments from Reviewable

@kaizencodes Will you have time to make the suggested changes? Or should I take this one over?

Review status: 15 of 19 files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

app/views/layouts/application.html.erb, line 6 at r4 (raw file):

there is a problem setting it to -bundle here, look at the build and hot configs. we push to the existing config and that syntax doesn't support the - char. we would have to rewrite the whole config.

app/views/layouts/application.html.erb, line 11 at r4 (raw file):

same as above

client/package.json, line 34 at r4 (raw file):

we set it so the config loader gets the dev paths from paths.yml

Comments from Reviewable

I don't understand the - issue.

Reviewed 4 of 4 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

Gemfile, line 41 at r6 (raw file):

gem "rails-html-sanitizer"
gem "react_on_rails", "8.0.0.beta.1"

beta.2

app/views/layouts/application.html.erb, line 6 at r4 (raw file):

I'm not following this comment.

Why can we support the "-"?

Comments from Reviewable

:lgtm:

Reviewed 11 of 11 files at r7.
Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

Comments from Reviewable

See #395