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.

Sign up for GitHub

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 shakapacker.yml as the secondary source for asset_host and relative_url_root #376

Merged
merged 11 commits into from
Dec 22, 2023
Merged

Add shakapacker.yml as the secondary source for asset_host and relative_url_root #376

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
30f70c9
Fix mistake in test title
ahangarha Nov 7, 2023
5f45082
Ensure string result from methods
ahangarha Nov 7, 2023
f4329de
Use asset_host in config file as the second priority
ahangarha Nov 7, 2023
7b83800
Update changelog
ahangarha Nov 7, 2023
7e00d32
Fix typo
ahangarha Nov 17, 2023
4a363ef
Remove any references to relative_url_root
ahangarha Nov 18, 2023
4b32eaa
Revert "Remove any references to relative_url_root"
ahangarha Dec 6, 2023
3dcebb0
Remove relative_url_root from config file
ahangarha Dec 7, 2023
7374cb5
Show deprecation message for `relateive_url_root`
ahangarha Dec 7, 2023
dd6484e
Update changelog
ahangarha Dec 7, 2023
663f294
Revert changes and return nil for asset_host and relative_url_root if…
ahangarha Dec 21, 2023
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: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,16 @@ _Please add entries here for your pull requests that are not yet released._
### Added
- Experimental support for other JS package managers using `package_json` gem [PR 349](https://github.com/shakacode/shakapacker/pull/349) by [G-Rath](https://github.com/g-rath).
- Support `hmr: only` configuration [PR 378](https://github.com/shakacode/shakapacker/pull/378) by [SimenB](https://github.com/SimenB).
- Use `config/shakapacker.yml` as the secondary source for `asset_host` and `relative_url_root` configurations [PR 376](https://github.com/shakacode/shakapacker/pull/376) by [ahangarha](https://github.com/ahangarha).

### Fixed
- Recommend `server` option instead of deprecated `https` option when `--https` is provided [PR 380](https://github.com/shakacode/shakapacker/pull/380) by [G-Rath](https://github.com/g-rath)
- Recompile assets on asset host change [PR 364](https://github.com/shakacode/shakapacker/pull/364) by [ahangarha](https://github.com/ahangarha).
- Add deprecation warning for `https` option in `shakapacker.yml` (use `server: 'https'` instead) [PR 382](https://github.com/shakacode/shakapacker/pull/382) by [G-Rath](https://github.com/g-rath).

### Deprecated
- The usage of relative_url_root is deprecated in Shakapacker and will be removed in v8. [PR 376](https://github.com/shakacode/shakapacker/pull/376) by [ahangarha](https://github.com/ahangarha).

## [v7.1.0] - September 30, 2023

### Added
Expand Down
5 changes: 5 additions & 0 deletions lib/install/config/shakapacker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ default: &default
# https://webpack.js.org/guides/build-performance/#avoid-production-specific-tooling
useContentHash: false

# Setting the asset host here will override Rails.application.config.asset_host.
# Here, you can set different asset_host per environment. Note that
# SHAKAPACKER_ASSET_HOST will override both configurations.
# asset_host: custom-path

development:
<<: *default
compile: true
Expand Down
12 changes: 10 additions & 2 deletions lib/shakapacker/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,19 @@ def fetch(key)
end
Copy link
Member

Choose a reason for hiding this comment

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

Ca fetch return a ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can. Internally, it runs fetch on a hash. So it can potentially return "" or " ". Though it is very unlikely someone deliberately pass key: "" in the config file.

Should we consider this in fetch as kind of normalization to ensure we either get a meaningful value or nil? I cannot think of any scenario where in the config we want to pass "" and not nil.


def asset_host
ENV.fetch("SHAKAPACKER_ASSET_HOST", ActionController::Base.helpers.compute_asset_host)
ENV.fetch(
"SHAKAPACKER_ASSET_HOST",
fetch(:asset_host) || ActionController::Base.helpers.compute_asset_host
Copy link
Member

Choose a reason for hiding this comment

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

how about fetch(:asset_host).presence to ensure not blank?

Copy link
Member

Choose a reason for hiding this comment

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

👍 but no change? why?

).to_s
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only concern with this PR is that I ensure the retuned value is a string in any case. This means if nothing is provided, we get "" (empty string).

That time, I thought it could help with simplification so that could only check asset_host.empty?.

I just wanted to raise this again to ensure it is not missed out in the reviews.


def relative_url_root
Copy link
Member

Choose a reason for hiding this comment

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

Is this a public API?

Do we need docs explaining this change?

What is the migration if somebody used this ENV: SHAKAPACKER_RELATIVE_URL_ROOT.

Is this OK as a feature level bump?

@G-Rath @ahangarha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not making it enough clear. I added more explanation in the commit message but it is not enough visible. So let's quote it here:

Remove any references to relative_url_root

Despite being added in webpacker, it never got used. There is no
functionality tied to this configuration and this commit, removes its
references in the code and tests.

and from my private discussion with Judah, he phrased it this way:

Looks like relative_url_root was added by rails/webpacker#1236, but instead of doing a follow-up PR to add the relative_url_root to the publicPath, they went with a public_root configuration instead (which publicPath does use) rails/webpacker#1848

So relative_url_root looks to be dead code.

So, we just inherited this extra unused configuration from Webpacker.
To my understanding, removing this shouldn't have any impact on any project (unless someone has made a customized shakapacker and used this configuration, but it is very unlikely.

I am not sure if we should keep carrying this code by deprecating it till the next major release. Mentioning this removal in the changelog wouldn't be bad (if needed).

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm I'm not quite following, but I think if relative_url_root is just dead code that's otherwise not actually causing harm, I'd prefer leaving it alone in this PR and then dealing with it separately.

We'll want to do a v8 for switching to package_json provided that goes well which I'd personally like to have out within the next 6 months so assuming others are happy with that timeline, it should be pretty fine to slap a deprecation warning on relative_url_root knowing it won't be hanging around for much longer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is also fine to me.

@justin808 I reverted the last commit (of removing relative_url_root).
We will handle the deprecation later.

If agreed, and there is no other required changes, we are good to go with merge and 7.2 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is relative_url_root actually being used though? overall I think if the plan is to deprecate this, we shouldn't change it at all unless it's actually broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I agree with you. I don't see any good reason for introducing a new public interface to Configuration for a deprecated feature.

So I will narrow down this PR to only cover asset_host and not relative_url_root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the actual moving of the logic into the configuration was done in #374. This PR was to add some fixes and improve tests. The PR title is misleading.

So I think now that all usages of relative_url_root goes through a method in Configuration, that method is the right place for showing a deprecation message.

ENV.fetch("SHAKAPACKER_RELATIVE_URL_ROOT", ActionController::Base.relative_url_root)
Shakapacker.puts_deprecation_message "The usage of relative_url_root is deprecated in Shakapacker and will be removed in v8."

ENV.fetch(
"SHAKAPACKER_RELATIVE_URL_ROOT",
fetch(:relative_url_root) || ActionController::Base.relative_url_root
).to_s
end

private
Expand Down
4 changes: 2 additions & 2 deletions spec/backward_compatibility_specs/compiler_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
end

it "accepts external env variables" do
expect(Webpacker.compiler.send(:webpack_env)["SHAKAPACKER_ASSET_HOST"]).to be nil
expect(Webpacker.compiler.send(:webpack_env)["SHAKAPACKER_RELATIVE_URL_ROOT"]).to be nil
expect(Webpacker.compiler.send(:webpack_env)["SHAKAPACKER_ASSET_HOST"]).to eq ""
ahangarha marked this conversation as resolved.
Show resolved Hide resolved
expect(Webpacker.compiler.send(:webpack_env)["SHAKAPACKER_RELATIVE_URL_ROOT"]).to eq ""

ENV["WEBPACKER_ASSET_HOST"] = "foo.bar"
ENV["WEBPACKER_RELATIVE_URL_ROOT"] = "/baz"
Expand Down
4 changes: 2 additions & 2 deletions spec/shakapacker/compiler_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
end

it "accepts external env variables" do
expect(Shakapacker.compiler.send(:webpack_env)["SHAKAPACKER_ASSET_HOST"]).to be nil
expect(Shakapacker.compiler.send(:webpack_env)["SHAKAPACKER_RELATIVE_URL_ROOT"]).to be nil
expect(Shakapacker.compiler.send(:webpack_env)["SHAKAPACKER_ASSET_HOST"]).to eq ""
expect(Shakapacker.compiler.send(:webpack_env)["SHAKAPACKER_RELATIVE_URL_ROOT"]).to eq ""

allow(ENV).to receive(:fetch).with("SHAKAPACKER_ASSET_HOST", nil).and_return("foo.bar")
allow(ENV).to receive(:fetch).with("SHAKAPACKER_RELATIVE_URL_ROOT", nil).and_return("/baz")
Expand Down
61 changes: 52 additions & 9 deletions spec/shakapacker/configuration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,11 +346,31 @@
expect(config.asset_host).to eq "custom_host.abc"
end

it "returns ActionController::Base.helpers.compute_asset_host if SHAKAPACKER_ASSET_HOST is not set" do
allow(ActionController::Base.helpers).to receive(:compute_asset_host).and_return("domain.abc")
allow(ENV).to receive(:fetch).with("SHAKAPACKER_ASSET_HOST", "domain.abc").and_return("domain.abc")
context "without SHAKAPACKER_ASSET_HOST set" do
it "returns asset_host in shakapacker.yml if set" do
expect(config).to receive(:fetch).with(:asset_host).and_return("value-in-config-file.com")
expect(ENV).to receive(:fetch).with("SHAKAPACKER_ASSET_HOST", "value-in-config-file.com").and_return("value-in-config-file.com")

expect(config.asset_host).to eq "domain.abc"
expect(config.asset_host).to eq "value-in-config-file.com"
end

context "without asset_host set in the shakapacker.yml" do
it "returns ActionController::Base.helpers.compute_asset_host if SHAKAPACKER_ASSET_HOST is not set" do
expect(config).to receive(:fetch).with(:asset_host).and_return(nil)
expect(ActionController::Base.helpers).to receive(:compute_asset_host).and_return("domain.abc")
allow(ENV).to receive(:fetch).with("SHAKAPACKER_ASSET_HOST", "domain.abc").and_return("domain.abc")

expect(config.asset_host).to eq "domain.abc"
end

context "without ActionController::Base.helpers.compute_asset_host returing any value" do
it "returns an empty string" do
Copy link
Member

Choose a reason for hiding this comment

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

why empty strings and not nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was the logic when I was implementing these changes: while calculating digest for the watched assets, I noticed that we may get nil and at that point, it was strange for me to get nil for asset_host. I either could convert asset_host to string to ensure in case of getting nil, we don't break the logic (for contamination). Later I thought it is better to get empty string right out of asset_host method if nothing is provided. (Though I forgot to remove the conversion to string in the mentioned code above).

Now I think it is better to return nil and handle nil when we want to use it. So I change these lines back to return nil and convert the value into string when we need it.

expect(ENV).to receive(:fetch).with("SHAKAPACKER_ASSET_HOST", nil).and_return(nil)

expect(config.asset_host).to eq ""
end
end
end
end
end

Expand All @@ -363,17 +383,40 @@
)
end

it "shows deprecation message" do
expect { config.relative_url_root }.to output(/deprecated/).to_stdout
end

it "returns the value of SHAKAPACKER_RELATIVE_URL_ROOT if set" do
allow(ENV).to receive(:fetch).with("SHAKAPACKER_RELATIVE_URL_ROOT", nil).and_return("custom_value")
expect(ENV).to receive(:fetch).with("SHAKAPACKER_RELATIVE_URL_ROOT", nil).and_return("custom_value")

expect(config.relative_url_root).to eq "custom_value"
end

it "returns ActionController::Base.helpers.compute_asset_host if SHAKAPACKER_RELATIVE_URL_ROOT is not set" do
allow(ActionController::Base).to receive(:relative_url_root).and_return("abcd")
allow(ENV).to receive(:fetch).with("SHAKAPACKER_RELATIVE_URL_ROOT", "abcd").and_return("abcd")
context "without SHAKAPACKER_RELATIVE_URL_ROOT set" do
it "returns relative_url_root in shakapacker.yml if set" do
expect(config).to receive(:fetch).with(:relative_url_root).and_return("value-in-config-file")
expect(ENV).to receive(:fetch).with("SHAKAPACKER_RELATIVE_URL_ROOT", "value-in-config-file").and_return("value-in-config-file")

expect(config.relative_url_root).to eq "value-in-config-file"
end

expect(config.relative_url_root).to eq "abcd"
context "without relative_url_root set in the shakapacker.yml" do
it "returns ActionController::Base.relative_url_root if SHAKAPACKER_RELATIVE_URL_ROOT is not set" do
expect(ActionController::Base).to receive(:relative_url_root).and_return("abcd")
expect(ENV).to receive(:fetch).with("SHAKAPACKER_RELATIVE_URL_ROOT", "abcd").and_return("abcd")

expect(config.relative_url_root).to eq "abcd"
end

context "without ActionController::Base.relative_url_root returing any value" do
it "returns an empty string" do
expect(ENV).to receive(:fetch).with("SHAKAPACKER_RELATIVE_URL_ROOT", nil).and_return(nil)

expect(config.relative_url_root).to eq ""
end
end
end
end
end
end