use webpacker_lite #387

kaizencodes · 2017-04-11T09:52:19Z

Integrate webpacker_lite

This change is

justin808 · 2017-04-11T10:14:04Z

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

kaizencodes · 2017-04-11T15:13:35Z

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

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

Previously, justin808 (Justin Gordon) wrote…

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

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

Previously, justin808 (Justin Gordon) wrote…

missing the stylesheet tag

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

Previously, justin808 (Justin Gordon) wrote…

why require and not import?

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

Previously, justin808 (Justin Gordon) wrote…

should this have a hash?

I'm not sure

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

Previously, justin808 (Justin Gordon) wrote…

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

Then we might not need the yaml at all.

Comments from Reviewable

kaizencodes · 2017-04-11T15:16:50Z

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

Previously, kaizencodes (Daniel Szatmari) wrote…

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

or make so the stylesheet helper includes nothing if hot reloading

Comments from Reviewable

justin808 · 2017-04-11T19:04:46Z

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

Previously, kaizencodes (Daniel Szatmari) wrote…

Then we might not need the yaml at all.

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

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

Previously, justin808 (Justin Gordon) wrote…

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.

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

justin808 · 2017-04-11T19:07:23Z

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

Previously, kaizencodes (Daniel Szatmari) wrote…

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.

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

Previously, kaizencodes (Daniel Szatmari) wrote…

or make so the stylesheet helper includes nothing if hot reloading

the latter!

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

Previously, kaizencodes (Daniel Szatmari) wrote…

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

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

@robwise?

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

Previously, kaizencodes (Daniel Szatmari) wrote…

I'm not sure

Yes it should!

otherwise browser won't know file changed!

Comments from Reviewable

robwise · 2017-04-11T19:32:33Z

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

Previously, justin808 (Justin Gordon) wrote…

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?

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

kaizencodes · 2017-04-12T09:08:42Z

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

Previously, justin808 (Justin Gordon) wrote…

Yes it should!

otherwise browser won't know file changed!

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

Previously, robwise (Rob Wise) wrote…

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

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

Comments from Reviewable

justin808 · 2017-04-12T09:14:48Z

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

Previously, kaizencodes (Daniel Szatmari) wrote…

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

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

Previously, kaizencodes (Daniel Szatmari) wrote…

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

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

justin808 · 2017-04-15T09:42:22Z

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

justin808 · 2017-04-15T09:42:58Z

@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

justin808 · 2017-04-30T19:53:21Z

config/webpack/paths.yml

+    - .jsx
+    # - .sass
+    - .scss
+    # - .css


@kaizencodes We should tune up these files to match the updates we made to the generator.

justin808 · 2017-05-03T09:29:39Z

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

justin808 · 2017-05-06T09:27:24Z

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

justin808 · 2017-05-07T08:58:20Z

client/package.json

@@ -31,12 +31,12 @@
    "build:production:client": "NODE_ENV=production webpack --config webpack.client.rails.build.config.js",
    "build:production:server": "NODE_ENV=production webpack --config webpack.server.rails.build.config.js",
    "build:client": "webpack --config webpack.client.rails.build.config.js",
-    "build:dev:client": "webpack -w --config webpack.client.rails.build.config.js",
-    "build:dev:server": "webpack -w --config webpack.server.rails.build.config.js",
+    "build:dev:client": "NODE_ENV=development webpack -w --config webpack.client.rails.build.config.js",


@kaizencodes Why are we setting the NODE_ENV for development?

justin808 · 2017-05-08T00:05:05Z

client/webpack.client.base.config.js

-    },
+    modules: [
+      paths.source,
+      paths.node_modules,


@robwise, Webpack guru, what should we change this to? No need to put this in paths.yml, right? per shakacode/react_on_rails#834

I would just hardcode the string 'node_modules' and call it a day, you don't have to add any other paths here

justin808 · 2017-05-08T00:05:23Z

config/webpack/paths.yml

+  assets: webpack
+  manifest: manifest.json
+  node_modules: client/node_modules
+  source: client/app


Probably not necessary. See shakacode/react_on_rails#834

kaizencodes · 2017-05-08T16:15:06Z

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

Previously, justin808 (Justin Gordon) wrote…

these should change to -bundle

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

Previously, justin808 (Justin Gordon) wrote…

add -bundle

same as above

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

Previously, justin808 (Justin Gordon) wrote…

@kaizencodes Why are we setting the NODE_ENV for development?

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

Comments from Reviewable

justin808 · 2017-05-10T10:49:11Z

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

Previously, kaizencodes (Daniel Szatmari) wrote…

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.

I'm not following this comment.

Why can we support the "-"?

Comments from Reviewable

- use webpack-merge to support - in bundle name - update react_on_rails version

justin808 · 2017-05-11T09:24:45Z

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

justin808 · 2017-05-29T08:40:53Z

See #395

use webpacker_lite

1b4d1dd

Fix server build manifest

7c3bacf

update to 7.1.0 beta

d1cccdd

justin808 reviewed Apr 30, 2017

View reviewed changes

cleanup paths.yml

f0d1fb3

justin808 reviewed May 7, 2017

View reviewed changes

update to 8.0.0 beta

dbaab43

justin808 reviewed May 8, 2017

View reviewed changes

justin808 mentioned this pull request May 8, 2017

add migration doc shakacode/react_on_rails#835

Closed

delete only dev bundles in Procfiles

1722989

justin808 mentioned this pull request May 10, 2017

Any feedback on React on Rails? sharetribe/sharetribe#2961

Closed

unify bundle names

7bf3a48

- use webpack-merge to support - in bundle name - update react_on_rails version

justin808 mentioned this pull request May 11, 2017

Kaizencodes daniel/use webpacker lite #390

Closed

justin808 closed this May 29, 2017

use webpacker_lite #387

use webpacker_lite #387

Uh oh!

Conversation

kaizencodes commented Apr 11, 2017 • edited by justin808 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justin808 commented Apr 11, 2017

Uh oh!

kaizencodes commented Apr 11, 2017

Uh oh!

kaizencodes commented Apr 11, 2017

Uh oh!

justin808 commented Apr 11, 2017

Uh oh!

justin808 commented Apr 11, 2017

Uh oh!

robwise commented Apr 11, 2017

Uh oh!

kaizencodes commented Apr 12, 2017

Uh oh!

justin808 commented Apr 12, 2017

Uh oh!

justin808 commented Apr 15, 2017

Uh oh!

justin808 commented Apr 15, 2017

Uh oh!

justin808 Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

justin808 commented May 3, 2017

Uh oh!

justin808 commented May 6, 2017

Uh oh!

justin808 May 7, 2017

Choose a reason for hiding this comment

Uh oh!

justin808 May 8, 2017

Choose a reason for hiding this comment

Uh oh!

robwise May 9, 2017

Choose a reason for hiding this comment

Uh oh!

justin808 May 8, 2017

Choose a reason for hiding this comment

Uh oh!

kaizencodes commented May 8, 2017

Uh oh!

justin808 commented May 10, 2017

Uh oh!

justin808 commented May 11, 2017

Uh oh!

justin808 commented May 29, 2017

Uh oh!

Uh oh!

kaizencodes commented Apr 11, 2017 •

edited by justin808

Loading