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

Improve lines insertion in installers #1445

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

ricardotk002
Copy link
Contributor

Maintaining empty strings at the beginning of each line to be inserted can be error prone (eg. #1422). In this contribution I'm using the optimize_indentation method from Rails::Generators to insert lines, this helps to indent the lines and add the line breaks automatically. Rails uses this very often on their generators.

I also wanted to add some tests for the installers, but since they do more than generators I don't know where to start. Any help will be appreciated.

Maintaining empty strings at the beginning of each line to be inserted
can be error prone. This helps to indent the lines automatically and
add the linebreak, something that Rails do very often on their generators.
@guilleiguaran guilleiguaran merged commit ad0312d into rails:master Apr 24, 2018
@ricardotk002 ricardotk002 deleted the improvements-on-installers branch April 24, 2018 17:50
ricardotk002 added a commit to ricardotk002/webpacker that referenced this pull request Apr 24, 2018
In rails#1445 I forgot to remove
some line breaks that are no longer needed, `optimize_indentation`
will strip those and add them again.
@grk
Copy link
Contributor

grk commented Apr 25, 2018

The optimize_indentation method is available only in Rails 5.2+, so unless the aim is to only support latest Rails, this change should be reverted.

@gauravtiwari
Copy link
Member

//cc @guilleiguaran

@ricardotk002
Copy link
Contributor Author

@guilleiguaran @grk I'm really sorry about this, I didn't notice that this method was too new.

What do you think about using the String#indent directly instead of the helper?

diff --git a/lib/install/coffee.rb b/lib/install/coffee.rb
index aa19835..ef8f65f 100644
--- a/lib/install/coffee.rb
+++ b/lib/install/coffee.rb
@@ -13,7 +13,7 @@ insert_into_file Rails.root.join("config/webpack/environment.js").to_s,
   before: "module.exports"
 
 say "Updating webpack paths to include .coffee file extension"
-insert_into_file Webpacker.config.config_path, optimize_indentation("- .coffee", 4), after: /extensions:\n/
+insert_into_file Webpacker.config.config_path, "- .coffee\n".indent(4), after: /extensions:\n/
 
 say "Copying the example entry file to #{Webpacker.config.source_entry_path}"
 copy_file "#{__dir__}/examples/coffee/hello_coffee.coffee",
diff --git a/lib/install/elm.rb b/lib/install/elm.rb
index 984d09f..d3803d7 100644
--- a/lib/install/elm.rb
+++ b/lib/install/elm.rb
@@ -26,7 +26,7 @@ run "yarn add --dev elm-hot-loader"
 run "yarn run elm package install -- --yes"
 
 say "Updating webpack paths to include .elm file extension"
-insert_into_file Webpacker.config.config_path, optimize_indentation("- .elm", 4), after: /extensions:\n/
+insert_into_file Webpacker.config.config_path, "- .elm\n".indent(4), after: /extensions:\n/
 
 say "Updating Elm source location"
 gsub_file "elm-package.json", /\"\.\"\n/,
diff --git a/lib/install/erb.rb b/lib/install/erb.rb
index 9fb8843..08c1dfa 100644
--- a/lib/install/erb.rb
+++ b/lib/install/erb.rb
@@ -13,7 +13,7 @@ insert_into_file Rails.root.join("config/webpack/environment.js").to_s,
   before: "module.exports"
 
 say "Updating webpack paths to include .erb file extension"
-insert_into_file Webpacker.config.config_path, optimize_indentation("- .erb", 4), after: /extensions:\n/
+insert_into_file Webpacker.config.config_path, "- .erb\n".indent(4), after: /extensions:\n/
 
 say "Copying the example entry file to #{Webpacker.config.source_entry_path}"
 copy_file "#{__dir__}/examples/erb/hello_erb.js.erb",
diff --git a/lib/install/react.rb b/lib/install/react.rb
index 299ea8e..0491fab 100644
--- a/lib/install/react.rb
+++ b/lib/install/react.rb
@@ -23,7 +23,7 @@ say "Copying react example entry file to #{Webpacker.config.source_entry_path}"
 copy_file "#{__dir__}/examples/react/hello_react.jsx", "#{Webpacker.config.source_entry_path}/hello_react.jsx"
 
 say "Updating webpack paths to include .jsx file extension"
-insert_into_file Webpacker.config.config_path, optimize_indentation("- .jsx", 4), after: /extensions:\n/
+insert_into_file Webpacker.config.config_path, "- .jsx\n".indent(4), after: /extensions:\n/
 
 say "Installing all react dependencies"
 run "yarn add react react-dom babel-preset-react prop-types"
diff --git a/lib/install/template.rb b/lib/install/template.rb
index 5867804..9dc2738 100644
--- a/lib/install/template.rb
+++ b/lib/install/template.rb
@@ -17,7 +17,7 @@ apply "#{__dir__}/binstubs.rb"
 
 say "Adding configurations"
 
-check_yarn_integrity_config = ->(value) { <<CONFIG }
+check_yarn_integrity_config = ->(value) { <<CONFIG.indent(2) }
 # Verifies that versions and hashed value of the package contents in the project's package.json
 config.webpacker.check_yarn_integrity = #{value}
 CONFIG
@@ -26,18 +26,18 @@ if Rails::VERSION::MAJOR >= 5
   environment check_yarn_integrity_config.call("true"), env: :development
   environment check_yarn_integrity_config.call("false"), env: :production
 else
-  inject_into_file "config/environments/development.rb", optimize_indentation(check_yarn_integrity_config.call("true"), 2), after: "Rails.application.configure do", verbose: false
-  inject_into_file "config/environments/production.rb", optimize_indentation(check_yarn_integrity_config.call("false"), 2), after: "Rails.application.configure do", verbose: false
+  inject_into_file "config/environments/development.rb", "\n#{check_yarn_integrity_config.call("true")}", after: "Rails.application.configure do", verbose: false
+  inject_into_file "config/environments/production.rb", "\n#{check_yarn_integrity_config.call("false")}", after: "Rails.application.configure do", verbose: false
 end
 
 if File.exists?(".gitignore")
-  append_to_file ".gitignore", optimize_indentation(<<-EOS)
-    /public/packs
-    /public/packs-test
-    /node_modules
-    yarn-debug.log*
-    .yarn-integrity
-  EOS
+  append_to_file ".gitignore", <<-EOS
+/public/packs
+/public/packs-test
+/node_modules
+yarn-debug.log*
+.yarn-integrity
+EOS
 end
 
 say "Installing all JavaScript dependencies"
diff --git a/lib/install/typescript.rb b/lib/install/typescript.rb
index 187f08a..cd2391f 100644
--- a/lib/install/typescript.rb
+++ b/lib/install/typescript.rb
@@ -31,10 +31,10 @@ say "Copying tsconfig.json to the Rails root directory for typescript"
 copy_file "#{__dir__}/examples/#{example_source}/tsconfig.json", "tsconfig.json"
 
 say "Updating webpack paths to include .ts file extension"
-insert_into_file Webpacker.config.config_path, optimize_indentation("- .ts", 4), after: /extensions:\n/
+insert_into_file Webpacker.config.config_path, "- .ts\n".indent(4), after: /extensions:\n/
 
 say "Updating webpack paths to include .tsx file extension"
-insert_into_file Webpacker.config.config_path, optimize_indentation("- .tsx", 4), after: /extensions:\n/
+insert_into_file Webpacker.config.config_path, "- .tsx\n".indent(4), after: /extensions:\n/
 
 say "Copying the example entry file to #{Webpacker.config.source_entry_path}"
 copy_file "#{__dir__}/examples/typescript/hello_typescript.ts",
diff --git a/lib/install/vue.rb b/lib/install/vue.rb
index b770cb8..c345d2e 100644
--- a/lib/install/vue.rb
+++ b/lib/install/vue.rb
@@ -22,7 +22,7 @@ insert_into_file Rails.root.join("config/webpack/environment.js").to_s,
   before: "module.exports"
 
 say "Updating webpack paths to include .vue file extension"
-insert_into_file Webpacker.config.config_path, optimize_indentation("- .vue", 4), after: /extensions:\n/
+insert_into_file Webpacker.config.config_path, "- .vue\n".indent(4), after: /extensions:\n/
 
 say "Copying the example entry file to #{Webpacker.config.source_entry_path}"
 copy_file "#{__dir__}/examples/vue/hello_vue.js",

I tried introducing a new helper that would mimic #optimize_indentation, but that seemed too much for a simple change.

I hope this helps, otherwise let me know and I can revert the commits I introduced.

@guilleiguaran
Copy link
Member

@ricardotk002 please send a PR replacing optimize_indentation with String#indent

ricardotk002 added a commit to ricardotk002/webpacker that referenced this pull request Apr 30, 2018
In rails#1445 I introduced the
method `#optimize_indentation`, but this was added in Rails 5.2
(rails/rails#30166) and Webpacker should
support Rails 4+.

In order to have the same benefit, I'm replacing the calls to the
method `#optimize_indentation` with `String#indent`, which is what the
original method uses.
ricardotk002 added a commit to ricardotk002/webpacker that referenced this pull request Apr 30, 2018
In rails#1445 I introduced the
method `#optimize_indentation`, but this was added in Rails 5.2
(rails/rails#30166) and Webpacker should
support Rails 4+.

In order to have the same benefit, I'm replacing the calls to the
method `#optimize_indentation` with the method it uses from behind:
`String#indent`
@ricardotk002
Copy link
Contributor Author

@guilleiguaran Sure! I submitted #1468 and ensured that we won't have this issue again. Thank you all for your help.

KingTiger001 added a commit to KingTiger001/Rails-web-pack-project that referenced this pull request Jan 15, 2023
In rails/webpacker#1445 I forgot to remove
some line breaks that are no longer needed, `optimize_indentation`
will strip those and add them again.
KingTiger001 added a commit to KingTiger001/Rails-web-pack-project that referenced this pull request Jan 15, 2023
In rails/webpacker#1445 I introduced the
method `#optimize_indentation`, but this was added in Rails 5.2
(rails/rails#30166) and Webpacker should
support Rails 4+.

In order to have the same benefit, I'm replacing the calls to the
method `#optimize_indentation` with the method it uses from behind:
`String#indent`
smartech7 pushed a commit to smartech7/ruby-webpacker that referenced this pull request Aug 4, 2023
In rails/webpacker#1445 I forgot to remove
some line breaks that are no longer needed, `optimize_indentation`
will strip those and add them again.
smartech7 pushed a commit to smartech7/ruby-webpacker that referenced this pull request Aug 4, 2023
In rails/webpacker#1445 I introduced the
method `#optimize_indentation`, but this was added in Rails 5.2
(rails/rails#30166) and Webpacker should
support Rails 4+.

In order to have the same benefit, I'm replacing the calls to the
method `#optimize_indentation` with the method it uses from behind:
`String#indent`
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.

4 participants