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

Add ruby 3 and node 16 & 18 in CI #1568

Merged
merged 16 commits into from
Aug 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/lint-js-and-ruby.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ jobs:
build:
strategy:
matrix:
ruby: [2.7]
node: [14]
ruby: [3]
node: [18]
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v2
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ jobs:
build-dummy-app-webpack-test-bundles:
strategy:
matrix:
ruby: [2.7]
node: [14]
ruby: [2.7, 3.2]
node: [16, 18]
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v2
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/package-js-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ jobs:
build:
strategy:
matrix:
node: [14]
node: [16, 18]
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v2
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/rspec-package-specs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ jobs:
build:
strategy:
matrix:
ruby: [2.7]
node: [14]
ruby: [2.7, 3.2]
node: [16, 18]
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v2
Expand Down
5 changes: 5 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,8 @@ RSpec/MultipleMemoizedHelpers:
Style/GlobalVars:
Exclude:
- 'spec/dummy/config/environments/development.rb'

RSpec/NoExpectationExample:
AllowedPatterns:
- ^expect_
- ^assert_
8 changes: 4 additions & 4 deletions Gemfile.development_dependencies
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

gem "shakapacker", "7.0.1"
gem "bootsnap", require: false
gem "rails", "~> 7.0"
gem "rails", "~> 7.0", ">= 7.0.1"
gem "sqlite3"
gem "sass-rails", "~> 6.0"
gem "uglifier"
Expand All @@ -29,9 +29,9 @@ group :development, :test do
gem "pry-doc"
gem "pry-rails"
gem "pry-rescue"
gem "rubocop", "1.14.0", require: false
gem "rubocop-performance", require: false
gem "rubocop-rspec", require: false
gem "rubocop", "~>1.56", require: false
gem "rubocop-performance", "~>1.18.0", require: false
gem "rubocop-rspec", "~>2.23.2", require: false
gem "scss_lint", require: false
gem "spring", "~> 4.0"
end
Expand Down
2 changes: 1 addition & 1 deletion lib/generators/react_on_rails/dev_tests_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def replace_prerender_if_server_rendering

hello_world_index = File.join(destination_root, "app", "views", "hello_world", "index.html.erb")
hello_world_contents = File.read(hello_world_index)
new_hello_world_contents = hello_world_contents.gsub(/prerender: false/,
new_hello_world_contents = hello_world_contents.gsub("prerender: false",
"prerender: true")

File.open(hello_world_index, "w+") { |f| f.puts new_hello_world_contents }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
# ActiveRecord::Migration.maintain_test_schema!

RSpec.configure do |config|
config.before(:each, type: :system, js: true) do
config.before(:each, js: true, type: :system) do
driven_by :selenium_chrome
end

Expand All @@ -49,7 +49,7 @@
ReactOnRails::TestHelper.configure_rspec_to_compile_assets(config)

# Remove this line if you"re not using ActiveRecord or ActiveRecord fixtures
config.fixture_path = "#{::Rails.root}/spec/fixtures"
config.fixture_path = "#{Rails.root}/spec/fixtures"

# If you"re not using ActiveRecord, or you"d prefer not to run each of your
# examples within a transaction, remove the following line or assign false
Expand Down
18 changes: 9 additions & 9 deletions lib/react_on_rails/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def check_autobundling_requirements_if_configured
end

def adjust_precompile_task
skip_react_on_rails_precompile = %w[no false n f].include?(ENV["REACT_ON_RAILS_PRECOMPILE"])
skip_react_on_rails_precompile = %w[no false n f].include?(ENV.fetch("REACT_ON_RAILS_PRECOMPILE", nil))

return if skip_react_on_rails_precompile || build_production_command.blank?

Expand Down Expand Up @@ -187,7 +187,7 @@ def error_if_using_webpacker_and_generated_assets_dir_not_match_public_output_pa
webpacker_public_output_path = ReactOnRails::WebpackerUtils.webpacker_public_output_path

if File.expand_path(generated_assets_dir) == webpacker_public_output_path.to_s
Rails.logger.warn("You specified generated_assets_dir in `config/initializers/react_on_rails.rb` "\
Rails.logger.warn("You specified generated_assets_dir in `config/initializers/react_on_rails.rb` " \
"with Webpacker. Remove this line from your configuration file.")
else
msg = <<~MSG
Expand Down Expand Up @@ -225,20 +225,20 @@ def configure_generated_assets_dirs_deprecation

if ReactOnRails::WebpackerUtils.using_webpacker?
webpacker_public_output_path = ReactOnRails::WebpackerUtils.webpacker_public_output_path
Rails.logger.warn "Error configuring config/initializers/react_on_rails. Define neither the "\
"generated_assets_dirs no the generated_assets_dir when using Webpacker. This is defined by "\
"public_output_path specified in webpacker.yml = #{webpacker_public_output_path}."
Rails.logger.warn "Error configuring config/initializers/react_on_rails. Define neither the " \
"generated_assets_dirs no the generated_assets_dir when using Webpacker. This is defined " \
"by public_output_path specified in webpacker.yml = #{webpacker_public_output_path}."
return
end

Rails.logger.warn "[DEPRECATION] ReactOnRails: Use config.generated_assets_dir rather than "\
Rails.logger.warn "[DEPRECATION] ReactOnRails: Use config.generated_assets_dir rather than " \
"generated_assets_dirs"
if generated_assets_dir.blank?
self.generated_assets_dir = generated_assets_dirs
else
Rails.logger.warn "[DEPRECATION] ReactOnRails. You have both generated_assets_dirs and "\
"generated_assets_dir defined. Define ONLY generated_assets_dir if NOT using Webpacker"\
" and define neither if using Webpacker"
Rails.logger.warn "[DEPRECATION] ReactOnRails. You have both generated_assets_dirs and " \
"generated_assets_dir defined. Define ONLY generated_assets_dir if NOT using Webpacker " \
"and define neither if using Webpacker"
end
end

Expand Down
6 changes: 3 additions & 3 deletions lib/react_on_rails/git_utils.rb
Copy link
Contributor Author

@ahangarha ahangarha Aug 22, 2023

Choose a reason for hiding this comment

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

Though I don't have any proposal for fixing the issue, this solution seems a bit hacky. It fixes our failing tests but it doesn't make sense to me. We can pass git_installed = false while we execute git status ....

Update:
I think there is another issue as well. We set git_installed to $CHILD_STATUS.success? before we run git status --porcelain. This means we do not capture suscess of the git command execution.

Maybe we should extract the entire git logic out of this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a form of mocking the git status for testing, because without it, we get a Cannot proxy frozen objects, rspec-mocks relies on proxies for method stubbing and expectations. error.

Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@

module ReactOnRails
module GitUtils
def self.uncommitted_changes?(message_handler)
def self.uncommitted_changes?(message_handler, git_installed = $CHILD_STATUS.success?)
return false if ENV["COVERAGE"] == "true"

status = `git status --porcelain`
return false if $CHILD_STATUS.success? && status.empty?
return false if git_installed && status&.empty?

error = if $CHILD_STATUS.success?
error = if git_installed
"You have uncommitted code. Please commit or stash your changes before continuing"
else
"You do not have Git installed. Please install Git, and commit your changes before continuing"
Expand Down
4 changes: 2 additions & 2 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def redux_store(store_name, props: {}, defer: false)
if defer
@registered_stores_defer_render ||= []
@registered_stores_defer_render << redux_store_data
"YOU SHOULD NOT SEE THIS ON YOUR VIEW -- Uses as a code block, like <% redux_store %> "\
"YOU SHOULD NOT SEE THIS ON YOUR VIEW -- Uses as a code block, like <% redux_store %> " \
"and not <%= redux store %>"
else
@registered_stores ||= []
Expand Down Expand Up @@ -245,7 +245,7 @@ def json_safe_and_pretty(hash_or_string)
return "{}" if hash_or_string.nil?

unless hash_or_string.is_a?(String) || hash_or_string.is_a?(Hash)
raise ReactOnRails::Error, "#{__method__} only accepts String or Hash as argument "\
raise ReactOnRails::Error, "#{__method__} only accepts String or Hash as argument " \
"(#{hash_or_string.class} given)."
end

Expand Down
4 changes: 2 additions & 2 deletions lib/react_on_rails/locales/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def convert
end

def generate_file(template, path)
result = ERB.new(template).result()
result = ERB.new(template).result
File.write(path, result)
end

Expand Down Expand Up @@ -157,7 +157,7 @@ def template_default
<<-JS.strip_heredoc
import { defineMessages } from 'react-intl';

const defaultLocale = \'#{default_locale}\';
const defaultLocale = '#{default_locale}';

const defaultMessages = defineMessages(#{@defaults});

Expand Down
2 changes: 1 addition & 1 deletion lib/react_on_rails/locales/to_js.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def template_default
<<-JS.strip_heredoc
import { defineMessages } from 'react-intl';

const defaultLocale = \'#{default_locale}\';
const defaultLocale = '#{default_locale}';

const defaultMessages = defineMessages(#{@defaults});

Expand Down
4 changes: 1 addition & 3 deletions lib/react_on_rails/packs_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,7 @@ def component_name_to_path(paths)
end

def common_component_to_path
common_components_paths = Dir.glob("#{components_search_path}/*").reject do |f|
CONTAINS_CLIENT_OR_SERVER_REGEX.match?(f)
end
common_components_paths = Dir.glob("#{components_search_path}/*").grep_v(CONTAINS_CLIENT_OR_SERVER_REGEX)
component_name_to_path(common_components_paths)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/react_on_rails/react_component/render_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def client_props
props.clone)
end

raise ReactOnRails::Error, "ReactOnRails: your rendering_props_extension module is missing the "\
raise ReactOnRails::Error, "ReactOnRails: your rendering_props_extension module is missing the " \
"required adjust_props_for_client_side_hydration method & can not be used"
end
props
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@

module ReactOnRails
module ServerRenderingPool
# rubocop:disable Metrics/ClassLength
class RubyEmbeddedJavaScript
# rubocop:enable Metrics/ClassLength
# rubocop:disable Metrics/ClassLength
class << self
def reset_pool
options = {
Expand Down Expand Up @@ -125,8 +124,8 @@ def read_bundle_js_code
File.read(server_js_file)
end
rescue StandardError => e
msg = "You specified server rendering JS file: #{server_js_file}, but it cannot be "\
"read. You may set the server_bundle_js_file in your configuration to be \"\" to "\
msg = "You specified server rendering JS file: #{server_js_file}, but it cannot be " \
"read. You may set the server_bundle_js_file in your configuration to be \"\" to " \
"avoid this warning.\nError is: #{e}"
raise ReactOnRails::Error, msg
end
Expand All @@ -145,15 +144,15 @@ def create_js_context
begin
if ReactOnRails.configuration.trace
Rails.logger.info do
"[react_on_rails] Created JavaScript context with file "\
"[react_on_rails] Created JavaScript context with file " \
"#{ReactOnRails::Utils.server_bundle_js_file_path}"
end
end
ExecJS.compile(base_js_code)
rescue StandardError => e
msg = "ERROR when compiling base_js_code! "\
"See file #{file_name} to "\
"correlate line numbers of error. Error is\n\n#{e.message}"\
msg = "ERROR when compiling base_js_code! " \
"See file #{file_name} to " \
"correlate line numbers of error. Error is\n\n#{e.message}" \
"\n\n#{e.backtrace.join("\n")}"
Rails.logger.error(msg)
trace_js_code_used("Error when compiling JavaScript code for the context.", base_js_code,
Expand Down Expand Up @@ -192,8 +191,8 @@ def execjs_timer_polyfills

def undefined_for_exec_js_logging(function_name)
if ReactOnRails.configuration.trace
"console.error('[React on Rails Rendering] #{function_name} is not defined for server rendering.');\n"\
" console.error(getStackTrace().join('\\n'));"
"console.error('[React on Rails Rendering] #{function_name} is not defined for server rendering.');\n " \
"console.error(getStackTrace().join('\\n'));"
else
""
end
Expand Down Expand Up @@ -235,6 +234,7 @@ def file_url_to_string(url)
raise ReactOnRails::Error, msg
end
end
# rubocop:enable Metrics/ClassLength
end
end
end
4 changes: 2 additions & 2 deletions lib/react_on_rails/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ def self.ensure_assets_compiled(webpack_assets_status_checker: nil,

unless @printed_once
puts
puts "====> React On Rails: Checking files in "\
"#{webpack_assets_status_checker.generated_assets_full_path} for "\
puts "====> React On Rails: Checking files in " \
"#{webpack_assets_status_checker.generated_assets_full_path} for " \
"outdated/missing bundles based on source_path #{source_path}"
puts
@printed_once = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ def all_compiled_assets
else
file_list = make_file_list(make_globs(generated_assets_full_path)).to_ary
puts "V" * 80
puts "Please define config.webpack_generated_files (array) so the test helper knows "\
"which files are required. If you are using webpacker, you typically need to only "\
puts "Please define config.webpack_generated_files (array) so the test helper knows " \
"which files are required. If you are using webpacker, you typically need to only " \
"include 'manifest.json'."
puts "Detected the possible following files to check for webpack compilation in "\
puts "Detected the possible following files to check for webpack compilation in " \
"#{generated_assets_full_path}"
puts file_list.join("\n")
puts "^" * 80
Expand Down
2 changes: 1 addition & 1 deletion lib/react_on_rails/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def self.smart_trim(str, max_length = 1000)
def self.find_most_recent_mtime(files)
files.reduce(1.year.ago) do |newest_time, file|
mt = File.mtime(file)
mt > newest_time ? mt : newest_time
[mt, newest_time].max
end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/react_on_rails/version_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def raise_differing_versions_warning
end

def raise_node_semver_version_warning
msg = "**ERROR** ReactOnRails: Your node package version for react-on-rails contains a "\
msg = "**ERROR** ReactOnRails: Your node package version for react-on-rails contains a " \
"^ or ~\n#{common_error_msg}"
raise ReactOnRails::Error, msg
end
Expand Down
2 changes: 1 addition & 1 deletion lib/tasks/assets.rake
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace :react_on_rails do
elsif build_production_command.methods.include?(:call)
build_production_command.call
else
msg = "ReactonRails.configuration.build_production_command is improperly configured. "\
msg = "ReactonRails.configuration.build_production_command is improperly configured. " \
"Value = #{build_production_command} with class #{build_production_command.class}"
puts Rainbow(msg).red
exit!(1)
Expand Down
Loading
Loading