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

Merged
merged 41 commits into from Dec 24, 2021

Conversation

gscarv13
Copy link
Contributor

@gscarv13 gscarv13 commented Nov 7, 2021

This PR contains:

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 as part of the generator
  3. Add Webpack config files

A demo project created with these current changes can be found here

And a quick walkthrough video here


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.

some comments

Reviewed 15 of 15 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @gscarv13)


.prettierignore, line 11 at r2 (raw file):

spec/react_on_rails/dummy-for-generators/app/javascript/bundles/HelloWorld/*
bundle/
lib/generators/react_on_rails/templates/base/base/config/webpack

I don't understand this change


lib/generators/react_on_rails/base_generator.rb, line 67 at r1 (raw file):

        base_files = %w[config/webpacker.yml]
        base_files.each { |file| copy_file("#{base_path}#{file}", file) }
      end

Please push up a sample public repo with the generator run.


lib/generators/react_on_rails/base_generator.rb, line 95 at r1 (raw file):

        puts "Adding dev dependencies"
        run "yarn add -D @pmmmwh/react-refresh-webpack-plugin fork-ts-checker-webpack-plugin react-refresh"

@Judahmeek we can leave the versions unlocked until the generator fails due to a new version breaking things.


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

// The source code including full typescript support is available at:
// https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/blob/master/babel.config.js

Should we invite people to join the slack to discuss installation/upgrade issues?


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

module.exports = function (api) {
  const validEnv = ['development', 'test', 'production'];

we could customize the version here: https://github.com/rails/webpacker/blob/master/package/babel/preset.js

we could invoke the function and modify the returned object


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;

name the style

bright


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

  # Cache manifest.json for performance
  cache_manifest: true

this file is installed by rails/webpacker. Not sure we should be overwriting it.

Copy link
Contributor Author

@gscarv13 gscarv13 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 16 files reviewed, 7 unresolved discussions (waiting on @gscarv13 and @justin808)


.prettierignore, line 11 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I don't understand this change

Prettier and ESLint rules are conflicting and breaking the CI, I thought it was better to ignore prettier in this case.


lib/generators/react_on_rails/base_generator.rb, line 67 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please push up a sample public repo with the generator run.

https://github.com/gscarv13/demo-update-webpacker-config


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

Previously, justin808 (Justin Gordon) wrote…

this file is installed by rails/webpacker. Not sure we should be overwriting it.

I added this initially because there are some small changes compared to the one that is installed by default, you can see the diff here https://www.diffchecker.com/ebXd6vc2

Maybe keep the original and mention the changes necessary to make it work on the tutorial?

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 2 of 2 files at r4, 11 of 11 files at r5, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gscarv13 and @justin808)


lib/generators/react_on_rails/base_generator.rb, line 98 at r5 (raw file):

        puts "Adding default javascript Rails dependencies"
        run " yarn add @rails/ujs turbolinks @rails/activestorage"

This makes no sense at all.

Seems totally unnecessary. Did you see https://github.com/shakacode/react_on_rails/blob/master/docs/guides/tutorial.md ?

Why are you adding these?


lib/generators/react_on_rails/base_generator.rb, line 101 at r5 (raw file):

        puts "Adding dev dependencies"
        run "yarn add -D @pmmmwh/react-refresh-webpack-plugin fork-ts-checker-webpack-plugin react-refresh"

base generator should not have TS

it's the basics


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

module.exports = function (api) {
  // eslint-disable-next-line global-require
  const defaultConfigFunc = require('@rails/webpacker/package/babel/preset.js');

why is the require inside of the function?

I think there is no reason it can't be outside.


lib/generators/react_on_rails/templates/base/base/Procfile.dev, line 5 at r5 (raw file):

rails: bundle exec rails s -p 3000
wp-client: bin/webpack-dev-server
wp-server: SERVER_BUNDLE_ONLY=yes bin/webpack --watch

check the red chars at the line ending

please configure editor correctly

@gscarv13 gscarv13 force-pushed the update-webpacker-config branch 2 times, most recently from d09941a to 6502884 Compare November 10, 2021 01:23
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 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gscarv13)


lib/generators/react_on_rails/base_generator.rb, line 67 at r1 (raw file):

Previously, gscarv13 (Gustavo Carvalho) wrote…

https://github.com/gscarv13/demo-update-webpacker-config

Please add this to the main PR comment.

Can you create a loom video walkthrough of the tutorial.md steps?


lib/generators/react_on_rails/base_generator.rb, line 98 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

This makes no sense at all.

Seems totally unnecessary. Did you see https://github.com/shakacode/react_on_rails/blob/master/docs/guides/tutorial.md ?

Why are you adding these?

OK

@gscarv13 please mark comments as resolved for me next time.


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

Previously, justin808 (Justin Gordon) wrote…

Should we invite people to join the slack to discuss installation/upgrade issues?

@gscarv13 can we have the install generator insert a template here?

That way we have ONE copy of the header.

https://dev.to/vinistock/understanding-and-writing-rails-generators-10h1
see the .tt files
https://arsfutura.com/magazine/diy-create-your-own-rails-generator/

template "service.erb", generator_path
that will copy the file and expand templates (erb)


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

Previously, gscarv13 (Gustavo Carvalho) wrote…

Maybe keep the original and mention the changes necessary to make it work on the tutorial?

You're 100% right. We should overwrite it if one is going to use the generator.


lib/generators/react_on_rails/templates/base/base/Procfile.dev-hmr, line 1 at r6 (raw file):

# Procfile for development using HMR

this is not right

you have this reversed and we should update the docs

procfile.dev ==> HMR ==> default
procfile.dev-static ==> static files, not default

Copy link
Contributor Author

@gscarv13 gscarv13 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 24 files reviewed, 6 unresolved discussions (waiting on @gscarv13 and @justin808)


lib/generators/react_on_rails/templates/base/base/Procfile.dev, line 5 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

check the red chars at the line ending

please configure editor correctly

Done.


lib/generators/react_on_rails/templates/base/base/Procfile.dev-hmr, line 1 at r6 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this is not right

you have this reversed and we should update the docs

procfile.dev ==> HMR ==> default
procfile.dev-static ==> static files, not default

Done.


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

Previously, justin808 (Justin Gordon) wrote…

name the style

bright

Done.


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

Previously, justin808 (Justin Gordon) wrote…

You're 100% right. We should overwrite it if one is going to use the generator.

Done.


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

Previously, justin808 (Justin Gordon) wrote…

@gscarv13 can we have the install generator insert a template here?

That way we have ONE copy of the header.

https://dev.to/vinistock/understanding-and-writing-rails-generators-10h1
see the .tt files
https://arsfutura.com/magazine/diy-create-your-own-rails-generator/

template "service.erb", generator_path
that will copy the file and expand templates (erb)

Done.

Copy link
Contributor Author

@gscarv13 gscarv13 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 24 files reviewed, 6 unresolved discussions (waiting on @justin808)


lib/generators/react_on_rails/base_generator.rb, line 67 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please add this to the main PR comment.

Can you create a loom video walkthrough of the tutorial.md steps?

Done.

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 15 of 15 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gscarv13)

@justin808
Copy link
Member

@gscarv13 Really fabulous!

For the CI failure, need to update the installer so that the new version of @rails/webpcker is used, > 6.0.0.rc.6 -

image

image

Skipping `rails webpacker:install` because `bundle install` was skipped.
To complete setup, you must run `bundle install` followed by `rails webpacker:install`.
cd /home/runner/work/react_on_rails/react_on_rails/gen-examples/examples/basic && touch .gitignore
cd /home/runner/work/react_on_rails/react_on_rails/gen-examples/examples/basic && rake webpacker:install

So we need to update that part to the ci.

rails new basic --skip-bundle --skip-spring --skip-git --skip-test-unit --skip-active-record

I think maybe we can run something like this after the rails install command, which I think installs the older version of webpacker.

bundle update webpacker --version 6.0.0.rc.6

@gscarv13 gscarv13 force-pushed the update-webpacker-config branch 2 times, most recently from ba4a999 to 1a6bf40 Compare November 16, 2021 15:35
@justin808
Copy link
Member

If we merge to master, we need to ship a 12.5.0 release and update the tutorial instructions to reflect that they apply to the 12.5.0 release of the gem.

Copy link
Contributor

@Judahmeek Judahmeek left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 21 files at r12, 9 of 9 files at r14, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gscarv13 and @justin808)

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 14 of 21 files at r12, 9 of 9 files at r14, 8 of 8 files at r15, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gscarv13)


Gemfile.development_dependencies, line 23 at r9 (raw file):

Previously, gscarv13 (Gustavo Carvalho) wrote…

The new update has some error that occurs when bundler is building the gem, has been reported on this issue. The issue mentions 0.5.0.pre but also happen on 0.5.0 also reported on this issue

Because of that error happening both, locally and on Github actions I thought it was a good idea to lock it for now.

remove


docs/guides/tutorial.md, line 73 at r15 (raw file):

```bash
bundle add webpacker --git=https://github.com/rails/webpacker.git
bundle add react_on_rails --version=12.4.0 --strict

Double-check. Do we need a new React on Rails version? 12.4 will have the old version of the generator.


lib/generators/react_on_rails/base_generator.rb, line 74 at r9 (raw file):

Previously, gscarv13 (Gustavo Carvalho) wrote…

rubyjs/mini_racer#218
Because of this current issue.

remove

but add comment to:

  1. tutorial
  2. console output from generator

rakelib/task_helpers.rb, line 17 at r9 (raw file):

Previously, gscarv13 (Gustavo Carvalho) wrote…

I tried removing it and using webpacker from git on the master branch, but the error persists.
Maybe needs to wait for the next release?

Let's fix in a followup.

// place any code here that is for test only
};

module.exports = webpackConfig(developmentEnvOnly);
Copy link
Member

Choose a reason for hiding this comment

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

@Judahmeek this file is for NODE_ENV=test for Jest testing, and not Rails, FYI.

So I don't see the value of this file. It could just be the default.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. I vaguely remembered that discussion, but I couldn't remember the exact reason.

I'll remove it.

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 4 of 6 files at r16, 2 of 2 files at r17, 4 of 5 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gscarv13)

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.

I'll merge and release soon.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gscarv13)

@justin808 justin808 merged commit 64f722f into shakacode:master Dec 24, 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.

None yet

3 participants