feat: make compilation stale on asset host change #364

ahangarha · 2023-10-04T12:32:47Z

Summary

This PR adds a feature for making digest strategy sensitive towards asset_host change. This way, by changing SHAKAPACKER_ASSET_HOST, we get new asset compilation.

This is an opt-in feature.

Pull Request checklist

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

Other Information

Closes #328

Judahmeek

LGTM. Just have one question.

lib/shakapacker/digest_strategy.rb

ahangarha · 2023-10-05T07:51:01Z

@Judahmeek
There was an issue with memoization. I fixed it. Now it only runs the digest algorithm once.

lib/install/config/shakapacker.yml

lib/shakapacker/digest_strategy.rb

ahangarha

Based on my comments, I wonder if we are testing the actual behavior of Shakapacker. In actual apps, I couldn't get any value for Shakapacker::Compiler.env except an empty hash.

lib/shakapacker/digest_strategy.rb

spec/shakapacker/digest_strategy_spec.rb

lib/shakapacker/digest_strategy.rb

spec/backward_compatibility_specs/digest_strategy_spec.rb

spec/shakapacker/digest_strategy_spec.rb

G-Rath · 2023-10-27T03:47:07Z

I think the problem here is that these are mincing config with env variables: SHAKAPACKER_ASSET_HOST gets set within a private instance method and the resulting env object is passed straight to Open3.capture3 so there's no way you can access it from the digest strategies (aka Shakapacker::Compiler.env["SHAKAPACKER_ASSET_HOST"] will never work).

What they do have is an instance of Shakapacker::Configuration, which feels like a suitable way to expose this anyway i.e. support asset_host within shakapacker.yml whose default value is ActionController::Base.helpers.compute_asset_host unless SHAKAPACKER_ASSET_HOST is set.

You should be able do that by adding this to Shakapacker::Configuration:

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

Then in the digest strategy and Shakapacker::Compiler#webpack_env you can do config.asset_host.

Also, I've not tested this but I'm pretty sure you can make this part of the digest returned by watched_files_digest rather than the filepath digest - I think that's nicer because it means those semantics are preserved and you also avoid growing the file name significantly which after a while will tend to annoy Windows.

justin808 · 2023-10-31T04:06:00Z

this needs rebasing. I merged #374

G-Rath · 2023-11-01T03:16:15Z

lib/shakapacker/digest_strategy.rb

@@ -53,7 +53,12 @@ def record_compilation_digest
      end

      def compilation_digest_path
-        config.cache_path.join("last-compilation-digest-#{Shakapacker.env}")
+        asset_host = Shakapacker.config.asset_host


can we try including this as part of the digest of the files themselves rather than the file path? that way the path will remain consistent and smaller, especially if in future there are other elements we decide we should be cachebusting on too

This is a good point, and I will try to implement it.
Thanks for the suggestion.

G-Rath · 2023-11-01T19:12:15Z

README.md

@@ -447,6 +447,13 @@ For more details see

 If you want to use live code reloading, or you have enough JavaScript that on-demand compilation is too slow, you'll need to run `./bin/shakapacker-dev-server`. This process will watch for changes in the relevant files, defined by `shakapacker.yml` configuration settings for `source_path`, `source_entry_path`, and `additional_paths`, and it will then automatically reload the browser to match. This feature is also known as [Hot Module Replacement](https://webpack.js.org/concepts/hot-module-replacement/).

+**Note:**


fwiw imo this is a bug not a feature since its reasonable to assume given how digest works that changing the asset host would result in the digest changing, and as such doesn't require explicit documentation (similarly, I don't think it does for mtime either because that clearly says its based on timestamps only)

justin808 · 2023-11-06T23:25:23Z

@ahangarha please make changes suggested by @G-Rath!

ahangarha · 2023-11-07T09:21:51Z

Regarding reviews:

Make asset_host changes affect the digest of the files rather than filenames from this comment.
Update the changelog and list this PR as a fix instead of a new feature from this comment.
No change in this PR regarding this comment since this should be done in another PR (Add shakapacker.yml as the secondary source for asset_host and relative_url_root #376).

lib/shakapacker/digest_strategy.rb

G-Rath

This is looking good to me, just needs the change to the readme removed and then should be good to go

lib/shakapacker/digest_strategy.rb

justin808 · 2023-11-23T02:15:42Z

LGTM

G-Rath

the readme change is still there, but happy to do a follow-up PR removing it instead of blocking this :)

ahangarha · 2023-11-24T18:34:58Z

the readme change is still there

Frankly, I don't know how that happened.
In any case, in resolving the merge conflict, I fixed this issue too.

G-Rath · 2023-11-24T19:21:33Z

README.md

@@ -447,6 +447,8 @@ For more details see

 If you want to use live code reloading, or you have enough JavaScript that on-demand compilation is too slow, you'll need to run `./bin/shakapacker-dev-server`. This process will watch for changes in the relevant files, defined by `shakapacker.yml` configuration settings for `source_path`, `source_entry_path`, and `additional_paths`, and it will then automatically reload the browser to match. This feature is also known as [Hot Module Replacement](https://webpack.js.org/concepts/hot-module-replacement/).

+**Note:** If you want to enforce recompilation on asset host change (for example, through `SHAKAPACKER_ASSET_HOST` environment variable), in addition to using digest strategy, you should set `compiler_strategy_asset_host_sensitive: true` in `config/shakapacker.yml`.


@ahangarha this looks like a previous doc update that was since removed - I think it would probably be best to just do a brand new commit manually undoing the changes to the readme rather than trying to do git revert since everything will be squashed anyway

Well...
I removed it manually.
Finally... 🙂

This reverts commit 8a5e8d9.

This reverts commit 272b36a.

ahangarha requested a review from Judahmeek October 4, 2023 14:30

ahangarha changed the title ~~WIP - feat: make compilation stale on asset host change~~ feat: make compilation stale on asset host change Oct 4, 2023

Judahmeek approved these changes Oct 5, 2023

View reviewed changes

lib/shakapacker/digest_strategy.rb Outdated Show resolved Hide resolved

ahangarha requested a review from Judahmeek October 5, 2023 07:51

justin808 reviewed Oct 15, 2023

View reviewed changes

lib/install/config/shakapacker.yml Outdated Show resolved Hide resolved

ahangarha requested a review from justin808 October 16, 2023 11:16

justin808 requested changes Oct 19, 2023

View reviewed changes

lib/shakapacker/digest_strategy.rb Outdated Show resolved Hide resolved

lib/shakapacker/digest_strategy.rb Outdated Show resolved Hide resolved

ahangarha commented Oct 25, 2023

View reviewed changes

lib/shakapacker/digest_strategy.rb Outdated Show resolved Hide resolved

spec/shakapacker/digest_strategy_spec.rb Outdated Show resolved Hide resolved

justin808 reviewed Oct 27, 2023

View reviewed changes

lib/shakapacker/digest_strategy.rb Outdated Show resolved Hide resolved

spec/backward_compatibility_specs/digest_strategy_spec.rb Show resolved Hide resolved

spec/shakapacker/digest_strategy_spec.rb Outdated Show resolved Hide resolved

ahangarha mentioned this pull request Oct 27, 2023

Move compiler configuration into configuration class #374

Merged

3 tasks

ahangarha force-pushed the make-compilation-stale-on-host-change branch from dc333f0 to 48a03bc Compare October 31, 2023 13:35

G-Rath suggested changes Nov 1, 2023

View reviewed changes

ahangarha requested review from G-Rath and justin808 November 7, 2023 09:35

justin808 reviewed Nov 7, 2023

View reviewed changes

lib/shakapacker/digest_strategy.rb Show resolved Hide resolved

ahangarha mentioned this pull request Nov 7, 2023

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

Merged

3 tasks

G-Rath suggested changes Nov 7, 2023

View reviewed changes

lib/shakapacker/digest_strategy.rb Show resolved Hide resolved

justin808 approved these changes Nov 23, 2023

View reviewed changes

G-Rath approved these changes Nov 23, 2023

View reviewed changes

ahangarha force-pushed the make-compilation-stale-on-host-change branch from 51f4d6d to 15c7a6d Compare November 24, 2023 18:33

G-Rath reviewed Nov 24, 2023

View reviewed changes

ahangarha requested a review from G-Rath November 24, 2023 21:31

G-Rath approved these changes Nov 24, 2023

View reviewed changes

ahangarha and others added 19 commits November 25, 2023 11:14

Apply logic for digest strategy

86cea46

Add backward compatibility

9910019

Memoize the genrerated host hash

ed68c79

Add documentation

a990c1a

Update changelog

3c7c79a

Fix memoization

013ea12

Remove unused arguments

d730b3a

Remove config for asset host sensitivity

b0c8184

Update documentation

d8f8e6a

simplify host_hash

54fce4d

Use config.asset_host

9f64f4a

Improve readability

ed6890f

Improve spec helper docs

f1e4e93

Stub ENV

e8c2c89

Make asset_host affect watched_files_digest rather file name

4d48491

Update docs in readme

a5fe8aa

Revert "Update docs in readme"

4664179

This reverts commit 8a5e8d9.

Revert "Update documentation"

b96ed06

This reverts commit 272b36a.

Remove the changes in the README file

6fad55e

ahangarha force-pushed the make-compilation-stale-on-host-change branch from 0bed557 to 6fad55e Compare November 25, 2023 07:45

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

justin808 deleted the make-compilation-stale-on-host-change branch December 1, 2023 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make compilation stale on asset host change #364

feat: make compilation stale on asset host change #364

ahangarha commented Oct 4, 2023 •

edited

Loading

Judahmeek left a comment

ahangarha commented Oct 5, 2023

ahangarha left a comment

G-Rath commented Oct 27, 2023

justin808 commented Oct 31, 2023

G-Rath Nov 1, 2023

ahangarha Nov 6, 2023

G-Rath Nov 1, 2023

justin808 commented Nov 6, 2023

ahangarha commented Nov 7, 2023 •

edited

Loading

G-Rath left a comment

justin808 commented Nov 23, 2023

G-Rath left a comment

ahangarha commented Nov 24, 2023

G-Rath Nov 24, 2023

ahangarha Nov 24, 2023

		@@ -447,6 +447,13 @@ For more details see

		If you want to use live code reloading, or you have enough JavaScript that on-demand compilation is too slow, you'll need to run `./bin/shakapacker-dev-server`. This process will watch for changes in the relevant files, defined by `shakapacker.yml` configuration settings for `source_path`, `source_entry_path`, and `additional_paths`, and it will then automatically reload the browser to match. This feature is also known as [Hot Module Replacement](https://webpack.js.org/concepts/hot-module-replacement/).

		Note:

		@@ -447,6 +447,8 @@ For more details see

		If you want to use live code reloading, or you have enough JavaScript that on-demand compilation is too slow, you'll need to run `./bin/shakapacker-dev-server`. This process will watch for changes in the relevant files, defined by `shakapacker.yml` configuration settings for `source_path`, `source_entry_path`, and `additional_paths`, and it will then automatically reload the browser to match. This feature is also known as [Hot Module Replacement](https://webpack.js.org/concepts/hot-module-replacement/).

		Note: If you want to enforce recompilation on asset host change (for example, through `SHAKAPACKER_ASSET_HOST` environment variable), in addition to using digest strategy, you should set `compiler_strategy_asset_host_sensitive: true` in `config/shakapacker.yml`.

feat: make compilation stale on asset host change #364

feat: make compilation stale on asset host change #364

Conversation

ahangarha commented Oct 4, 2023 • edited Loading

Summary

Pull Request checklist

Other Information

Judahmeek left a comment

Choose a reason for hiding this comment

ahangarha commented Oct 5, 2023

ahangarha left a comment

Choose a reason for hiding this comment

G-Rath commented Oct 27, 2023

justin808 commented Oct 31, 2023

G-Rath Nov 1, 2023

Choose a reason for hiding this comment

ahangarha Nov 6, 2023

Choose a reason for hiding this comment

G-Rath Nov 1, 2023

Choose a reason for hiding this comment

justin808 commented Nov 6, 2023

ahangarha commented Nov 7, 2023 • edited Loading

G-Rath left a comment

Choose a reason for hiding this comment

justin808 commented Nov 23, 2023

G-Rath left a comment

Choose a reason for hiding this comment

ahangarha commented Nov 24, 2023

G-Rath Nov 24, 2023

Choose a reason for hiding this comment

ahangarha Nov 24, 2023

Choose a reason for hiding this comment

ahangarha commented Oct 4, 2023 •

edited

Loading

ahangarha commented Nov 7, 2023 •

edited

Loading