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

Update the install generator to add Webpacker configuration, Add ReScript component and Fix Issue #1397 #1399

Closed

Conversation

gscarv13
Copy link
Contributor

@gscarv13 gscarv13 commented Oct 23, 2021

This PR contains:

Generator update

Updated the generator to add Webpack configuration on rails generate react_on_rails:install

  1. Add React, Typescript, and CSS modules dependencies to the generator
  2. Add Babel config and webpacker.yml to the
  3. Add Webpack config files

ReScript Example

Display error when SSR config is missing


This change is Reviewable

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Really great work. Let's discuss this on zoom!

"bs-dependencies": [
"@rescript/react"
]
}
Copy link
Member

Choose a reason for hiding this comment

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

@gscarv13 this means you're missing a return at the end of the file.

@@ -0,0 +1,103 @@
module.exports = function (api) {
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be hard to maintain.

Any thoughts on how we can rely more on rails/webpacker's defaults and then maybe modify those?

@justin808
Copy link
Member

Display error when SSR config is missing
Fix issue If config.server_bundle_js_file is blank, nil, commented out, and prerendering is done, page request hangs #1397

Please make separate PR.

@justin808
Copy link
Member

@justin808
Copy link
Member

/spec/dummy webpack config should roughly match the generator which should roughly match https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 27 of 28 files at r1, all commit messages.
Reviewable status: 27 of 28 files reviewed, 16 unresolved discussions (waiting on @gscarv13 and @justin808)


lib/generators/react_on_rails/templates/base/base/babel.config.js, line 1 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

This is going to be hard to maintain.

Any thoughts on how we can rely more on rails/webpacker's defaults and then maybe modify those?

Let's leave a comment at top indicating the source of this file, https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh


lib/generators/react_on_rails/templates/base/base/app/javascript/bundles/HelloWorld/components/HelloWorld.module.css, line 2 at r1 (raw file):

.bold {
    color: green;

bold and color green?

maybe call the style "bright"?

we can add the font-weight: bold as well


lib/generators/react_on_rails/templates/base/base/app/javascript/bundles/HelloWorld/components/HelloWorldServer.js, line 5 at r1 (raw file):

// For example, if using React-Router, we'd have the SSR setup here.

export default HelloWorld;

weird characters at the end of the line


lib/generators/react_on_rails/templates/base/base/config/webpacker.yml, line 1 at r1 (raw file):

# Note: You must restart bin/webpack-dev-server for changes to take effect

leave comment regarding source came from https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh


lib/generators/react_on_rails/templates/base/base/config/webpack/development.js, line 1 at r1 (raw file):

process.env.NODE_ENV = process.env.NODE_ENV || 'development'

for all these copied files, leave comment indicating source file in https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/


lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js, line 69 at r1 (raw file):

  // the client build will handle exporting CSS.
  // replace file-loader with null-loader
  const rules = serverWebpackConfig.module.rules;

we can add doc resource comment:

https://webpack.js.org/configuration/module/#modulerules


lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js, line 105 at r1 (raw file):

  // TODO: DELETE NEXT 2 LINES
  // Critical due to https://github.com/rails/webpacker/pull/2644
  // delete serverWebpackConfig.devServer

these need to be deleted ^^^^ (both repos)


lib/react_on_rails/helper.rb, line 58 at r1 (raw file):

    def react_component(component_name, options = {})
      config_server_bundle_js = ReactOnRails.configuration.server_bundle_js_file
      if options[:prerender] == true && (config_server_bundle_js.nil? || config_server_bundle_js == "")

use config_server_bundle.js.blank?


lib/react_on_rails/helper.rb, line 65 at r1 (raw file):

          Read more at https://www.shakacode.com/react-on-rails/docs/guides/react-server-rendering/
        MSG
        raise ReactOnRails::Error, msg

separate PR


rakelib/task_helpers.rb, line 3 at r1 (raw file):

# frozen_string_literal: true

require "bundler"

why was this needed?

how do you reproduce the issue that caused you to add this?


spec/dummy/bsconfig.json, line 24 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@gscarv13 this means you're missing a return at the end of the file.

@alexfedoseev can you confirm this is a good basic rescript configuration for React on Rails projects?


spec/dummy/Procfile.dev, line 6 at r1 (raw file):


# Bundle ReScript .res files
rescript: yarn re:start

We need to make sure that assets precompile will build.

RAILS_ENV=production NODE_ENV=production rake assets:precompile ==> then launch app rails -e production

We should add a Procfile with just that one line to start the rails server and comments that suggest how to build the assets.


spec/dummy/package.json, line 61 at r1 (raw file):

    "build:dev:watch": "RAILS_ENV=development NODE_ENV=development bin/webpack --watch",
    "build:clean": "rm -rf public/webpack || true",
    "re:build": "rescript build",

this needs to be called via assets:precompile


spec/dummy/client/app/components/HelloWorldReScript.bs.js, line 29 at r1 (raw file):

exports.make = make;
/* react Not a pure module */

Remove.

This file needs to be ignored via .gitignore.


spec/dummy/client/app/packs/client-bundle.js, line 14 at r1 (raw file):

import PureComponentWrappedInFunction from '../components/PureComponentWrappedInFunction';

import {make as HelloWorldReScript} from '../components/HelloWorldReScript.bs';

Add a JS component file that does this line above and exports the plain JS React component.

Then import that here.


spec/dummy/config/routes.rb, line 11 at r1 (raw file):

  get "client_side_hello_world" => "pages#client_side_hello_world"
  get "client_side_rescript_hello_world" => "pages#client_side_rescript_hello_world"

need a unit test to check this route works.

@justin808
Copy link
Member

justin808 commented Nov 1, 2021

  1. CI needs to pass
  2. Document steps from clean git clone:
  • use the generator to build a rescript app
  • run app in spec/dummy showing rescript component
  • run test for new rescript component.
  1. Update the basic tutorial given the updated generator https://github.com/shakacode/react_on_rails/blob/master/docs/guides/tutorial.md

@gscarv13 gscarv13 force-pushed the gscarv13/update-generator-config branch from f1300ff to 0864825 Compare November 2, 2021 19:29
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @gscarv13)


lib/generators/react_on_rails/base_generator.rb, line 40 at r2 (raw file):

      end

      def copy_webpack_config

Generator changes should be separate PR.


lib/generators/react_on_rails/templates/base/base/babel.config.js, line 1 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Let's leave a comment at top indicating the source of this file, https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh

DONE


lib/generators/react_on_rails/templates/base/base/babel.config.js, line 11 at r2 (raw file):

  const isDevelopmentEnv = api.env('development')
  const isProductionEnv = api.env('production')
  const isTestEnv = api.env('test')

If changing this file, change also https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/blob/master/babel.config.js ?


lib/generators/react_on_rails/templates/base/base/config/webpack/development.js, line 11 at r1 (raw file):

  const isWebpackDevServer = process.env.WEBPACK_DEV_SERVER

very important


lib/generators/react_on_rails/templates/base/base/config/webpack/development.js, line 19 at r1 (raw file):

    const ReactRefreshWebpackPlugin = require('@pmmmwh/react-refresh-webpack-plugin')
    clientWebpackConfig.plugins.push(
      new ReactRefreshWebpackPlugin({

Needed for HMR


lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js, line 1 at r2 (raw file):

/* eslint-disable no-unused-vars */

dont' do that for the whole file, maybe a single line.


lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js, line 2 at r2 (raw file):

/* eslint-disable no-unused-vars */
/* eslint-disable no-param-reassign */

don't do that

@gscarv13
Copy link
Contributor Author

gscarv13 commented Nov 3, 2021

This PR has been separated into 3:

@gscarv13 gscarv13 closed this Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants