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

Read package.json from APP_ROOT or Rails.root, not cwd #520

Merged
merged 1 commit into from
Oct 18, 2024
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Changes since the last non-beta release.
### Changed
- Changed internal `require`s to `require_relative` to make code less dependent on the load path. [PR 516](https://github.com/shakacode/shakapacker/pull/516) by [tagliala](https://github.com/tagliala).

### Fixed
- Fix error when rails environment is required from outside the rails root directory [PR 520](https://github.com/shakacode/shakapacker/pull/520)

## [v8.0.2] - August 28, 2024

### Fixed
Expand Down
15 changes: 13 additions & 2 deletions lib/shakapacker/utils/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class Error < StandardError; end

# Emits a warning if it's not obvious what package manager to use
def self.error_unless_package_manager_is_obvious!
return unless PackageJson.read.fetch("packageManager", nil).nil?
return unless PackageJson.read(rails_root).fetch("packageManager", nil).nil?

guessed_binary = guess_binary

Expand All @@ -35,7 +35,7 @@ def self.error_unless_package_manager_is_obvious!
#
# @return [String]
def self.guess_binary
MANAGER_LOCKS.find { |_, lock| File.exist?(lock) }&.first || "npm"
MANAGER_LOCKS.find { |_, lock| File.exist?(rails_root.join(lock)) }&.first || "npm"
end

# Guesses the version of the package manager to use by calling `<manager> --version`
Expand All @@ -53,6 +53,17 @@ def self.guess_version

stdout.chomp
end

private
def self.rails_root
if defined?(APP_ROOT)
Pathname.new(APP_ROOT)
elsif defined?(Rails)
Rails.root
else
raise "can only be called from a rails environment or with APP_ROOT defined"
end
end
end
end
end
60 changes: 59 additions & 1 deletion spec/shakapacker/utils_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def exitstatus
context "when there is a #{lock}" do
before do
allow(Open3).to receive(:capture3).and_return(["1.2.3\n", "", Struct::Status.new(0)])
allow(Rails).to receive(:root).and_return(Pathname.new("."))
end

it "raises an error about setting 'packageManager' for #{manager}" do
Expand All @@ -63,10 +64,35 @@ def exitstatus
MSG
end
end

context "when lockfile is in Rails.root, but pwd is different" do
before do
allow(Open3).to receive(:capture3).and_return(["1.2.3\n", "", Struct::Status.new(0)])
end

it "raises an error about setting 'packageManager' for #{manager}" do
rails_root = Pathname.new("rails_root_#{lock}")
FileUtils.mkdir_p(rails_root)
allow(Rails).to receive(:root).and_return(rails_root)

File.write(rails_root.join("package.json"), {}.to_json)
FileUtils.touch(rails_root.join(lock))

expect { Shakapacker::Utils::Manager.error_unless_package_manager_is_obvious! }.to raise_error(Shakapacker::Utils::Manager::Error, <<~MSG)
You don't have "packageManager" set in your package.json
meaning that Shakapacker will use npm but you've got a #{lock}
file meaning you probably want to be using #{manager} instead.

To make this happen, set "packageManager" in your package.json to #{manager}@1.2.3
MSG
end
end
marvinthepa marked this conversation as resolved.
Show resolved Hide resolved
end
end

describe "~guess_binary" do
before { allow(Rails).to receive(:root).and_return(Pathname.new(".")) }

Shakapacker::Utils::Manager::MANAGER_LOCKS.each do |manager, lock|
context "when a #{lock} exists" do
before { FileUtils.touch(lock) }
Expand All @@ -75,6 +101,19 @@ def exitstatus
expect(Shakapacker::Utils::Manager.guess_binary).to eq manager
end
end

context "when lockfile is in Rails.root, but pwd is different" do
before do
rails_root = Pathname.new("rails_root_#{lock}")
FileUtils.mkdir_p(rails_root)
FileUtils.touch(rails_root.join(lock))
allow(Rails).to receive(:root).and_return(rails_root)
end

it "guesses #{manager}" do
expect(Shakapacker::Utils::Manager.guess_binary).to eq manager
end
end
marvinthepa marked this conversation as resolved.
Show resolved Hide resolved
end

context "when there is no lockfile" do
Expand All @@ -91,7 +130,25 @@ def exitstatus

Shakapacker::Utils::Manager::MANAGER_LOCKS.each do |manager, lock|
context "when a #{lock} exists" do
before { FileUtils.touch(lock) }
before do
FileUtils.touch(lock)
allow(Rails).to receive(:root).and_return(Pathname.new("."))
end

it "calls #{manager} with --version" do
Shakapacker::Utils::Manager.guess_version

expect(Open3).to have_received(:capture3).with("#{manager} --version")
end
end

marvinthepa marked this conversation as resolved.
Show resolved Hide resolved
context "when lockfile is in Rails.root, but pwd is different" do
before do
rails_root = Pathname.new("rails_root_#{lock}")
FileUtils.mkdir_p(rails_root)
FileUtils.touch(rails_root.join(lock))
allow(Rails).to receive(:root).and_return(rails_root)
end
Comment on lines +145 to +151
Copy link

@coderabbitai coderabbitai bot Oct 9, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use shared context for consistent test setup

The context "when lockfile is in Rails.root, but pwd is different" in the guess_version method repeats code found in other tests. Implementing a shared context ensures consistent setup and reduces the likelihood of errors due to code duplication.

Copy link
Contributor Author

@marvinthepa marvinthepa Oct 9, 2024

Choose a reason for hiding this comment

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

If a human reviewer wants me to extract a shared context for something as simple as this, I am more than willing to oblige.

IMVHO, tests profit from a little bit of duplication if it makes the logic easier to follow. Especially rspec shared contexts sometimes make tests unecessarily complicated.

I saw some duplication in the existing code and thought this was the style of the project, but feel free to correct me if I am wrong.

Copy link

Choose a reason for hiding this comment

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

@marvinthepa, thank you for sharing your perspective. I understand that you prefer some duplication in tests for clarity, and that using shared contexts might make tests unnecessarily complicated. I'll keep that in mind for future reviews.

 (\_/)
 (•_•)  *pondering thoughtfully*
 /   \


✏️ Learnings added
Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:145-151
Timestamp: 2024-10-09T10:52:40.755Z
Learning: In tests, prefer some duplication if it makes the logic easier to follow. Avoid using RSpec shared contexts if they make tests unnecessarily complicated.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:68-89
Timestamp: 2024-10-09T10:47:17.620Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, the code within the context `"when lockfile is in Rails.root, but pwd is different"` is not duplicated for each package manager.


it "calls #{manager} with --version" do
Shakapacker::Utils::Manager.guess_version
Expand All @@ -110,6 +167,7 @@ def exitstatus
context "when the command errors" do
before do
allow(Open3).to receive(:capture3).and_return(["", "oh noes!", Struct::Status.new(1)])
allow(Rails).to receive(:root).and_return(Pathname.new("."))
end

it "raises an error" do
Expand Down