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

ahangarha · 2023-11-07T14:14:21Z

Summary

This is a follow-up PR, implementing what has been discussed here.

Pull Request checklist

Add/update test to cover these changes
Update documentation
Update CHANGELOG file

Note

This PR is made on top of make-compilation-stale-on-host-change branch and is supposed to get merged after the PR #364 merge.

Concerns

Do we actually use relative_url_path? It seems this is a leftover from some old feature which is now completely removed from Shakapacker. If so, we should remove it.

justin808 · 2023-11-17T00:55:48Z

@G-Rath I think we just need one PR for all this work.

@ahangarha if @G-Rath agrees, please consolidate the comments into this PR and change the base to main/master.

G-Rath · 2023-11-17T00:57:02Z

I think it's useful giving it a dedicated changelog entry because this is a feature whereas the other PR is a bug

ahangarha · 2023-11-17T10:05:14Z

I initially aimed to add this to the previous PR but then I thought it better to keep it separate to make a decision about it independently.

Since this PR is on top of the #364 PR, I would propose merging that PR first and then merging this one. This is why I made this PR as a draft to prevent merging.

justin808

Seems like we need

Migration instructions
Updated README?

CC: @ahangarha @G-Rath

justin808 · 2023-12-05T04:32:16Z

lib/shakapacker/configuration.rb

-    ENV.fetch("SHAKAPACKER_ASSET_HOST", ActionController::Base.helpers.compute_asset_host)
-  end
-
-  def relative_url_root


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

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).

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

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.

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

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.

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.

G-Rath

Personally I'd prefer the relative root changes to be done in a dedicated PR, but this seems fine enough to land

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.

This reverts commit 2677f81. We will address it later.

ahangarha · 2023-12-18T16:03:09Z

lib/shakapacker/configuration.rb

  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
+    ).to_s
  end


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.

justin808 · 2023-12-21T08:35:59Z

lib/shakapacker/configuration.rb

-    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


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

👍 but no change? why?

justin808 · 2023-12-21T08:36:37Z

lib/shakapacker/configuration.rb

@@ -117,11 +117,19 @@ def fetch(key)
  end


Ca fetch return a ""?

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.

spec/backward_compatibility_specs/compiler_spec.rb

justin808 · 2023-12-21T08:37:23Z

spec/shakapacker/configuration_spec.rb

+        end
+
+        context "without ActionController::Base.helpers.compute_asset_host returing any value" do
+          it "returns an empty string" do


why empty strings and not nil?

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.

… not provided

ahangarha changed the base branch from master to make-compilation-stale-on-host-change November 7, 2023 14:15

ahangarha mentioned this pull request Nov 7, 2023

feat: make compilation stale on asset host change #364

Merged

3 tasks

ahangarha marked this pull request as draft November 7, 2023 14:39

ahangarha requested a review from Judahmeek November 8, 2023 13:33

ahangarha self-assigned this Nov 8, 2023

ahangarha changed the base branch from make-compilation-stale-on-host-change to master November 17, 2023 10:05

ahangarha changed the title ~~Add asset_host and relative_url_path to config file~~ Add asset_host and relative_url_root to config file Nov 17, 2023

ahangarha force-pushed the fix-asset-host-and-relative-url-path-config branch 4 times, most recently from a627eb4 to 903b659 Compare November 25, 2023 07:45

ahangarha marked this pull request as ready for review December 2, 2023 20:07

ahangarha force-pushed the fix-asset-host-and-relative-url-path-config branch from b5fef8b to 2677f81 Compare December 3, 2023 08:46

justin808 requested changes Dec 5, 2023

View reviewed changes

G-Rath approved these changes Dec 7, 2023

View reviewed changes

ahangarha changed the title ~~Add asset_host and relative_url_root to config file~~ Add shakapacker.yml as the secondary source for asset_host and relative_url_root Dec 9, 2023

ahangarha added 10 commits December 18, 2023 19:07

Fix mistake in test title

30f70c9

Ensure string result from methods

5f45082

Use asset_host in config file as the second priority

f4329de

Update changelog

7b83800

Fix typo

7e00d32

Remove any references to relative_url_root

4a363ef

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.

Revert "Remove any references to relative_url_root"

4b32eaa

This reverts commit 2677f81. We will address it later.

Remove relative_url_root from config file

3dcebb0

Show deprecation message for relateive_url_root

7374cb5

Update changelog

dd6484e

ahangarha force-pushed the fix-asset-host-and-relative-url-path-config branch from 9610fe8 to dd6484e Compare December 18, 2023 15:41

ahangarha commented Dec 18, 2023

View reviewed changes

justin808 reviewed Dec 21, 2023

View reviewed changes

Revert changes and return nil for asset_host and relative_url_root if…

663f294

… not provided

ahangarha requested a review from justin808 December 21, 2023 15:41

justin808 merged commit 1cb8e16 into master Dec 22, 2023
82 checks passed

justin808 deleted the fix-asset-host-and-relative-url-path-config branch December 22, 2023 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ahangarha commented Nov 7, 2023 •

edited

Loading

justin808 commented Nov 17, 2023

G-Rath commented Nov 17, 2023

ahangarha commented Nov 17, 2023 •

edited

Loading

justin808 left a comment

justin808 Dec 5, 2023

ahangarha Dec 5, 2023

G-Rath Dec 5, 2023

ahangarha Dec 6, 2023

G-Rath Dec 6, 2023

ahangarha Dec 7, 2023

ahangarha Dec 7, 2023

G-Rath left a comment

ahangarha Dec 18, 2023

justin808 Dec 21, 2023

justin808 Dec 22, 2023

justin808 Dec 21, 2023

ahangarha Dec 21, 2023

justin808 Dec 21, 2023

ahangarha Dec 21, 2023

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

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

Conversation

ahangarha commented Nov 7, 2023 • edited Loading

Summary

Pull Request checklist

Note

Concerns

justin808 commented Nov 17, 2023

G-Rath commented Nov 17, 2023

ahangarha commented Nov 17, 2023 • edited Loading

justin808 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahangarha commented Nov 7, 2023 •

edited

Loading

ahangarha commented Nov 17, 2023 •

edited

Loading