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

Allow app to opt out of precompiling activestorage js assets #43967

Merged
merged 1 commit into from Jan 18, 2022

Conversation

jlestavel
Copy link
Contributor

Summary

Since PR #42895, Active Storage adds 2 JS assets to precompile:

initializer "active_storage.asset" do
if Rails.application.config.respond_to?(:assets)
Rails.application.config.assets.precompile += %w( activestorage activestorage.esm )
end
end

It's currently not possible to opt out. Our app does not use this Active Storage JS and it causes some issues with Sprockets and Uglifier. It would be helpful if we could disable this assets addition in the config.

Other Information

Action Cable has the same behavior of adding assets to precompile (introduced in #42856), but as you can see below it can be disabled:

initializer "action_cable.asset" do
config.after_initialize do |app|
if Rails.application.config.respond_to?(:assets) && app.config.action_cable.precompile_assets
Rails.application.config.assets.precompile += %w( actioncable.js actioncable.esm.js )
end
end
end

Specifically, this opt-out option was introduced by DHH in 3fd4185 so basically I mimicked this commit.

@jlestavel
Copy link
Contributor Author

@dhh I am taking the liberty of pinging you, as you're the one who introduced those new assets (and also the one who added the optout option for Action Cable ones). And @rafaelfranca because you were tagged in 3fd4185 (which is basically the same as this PR) so I guess you may be interested by this one

@ghiculescu
Copy link
Member

It looks good. Could you rebase?

@jlestavel jlestavel force-pushed the activestorage_assets_precompile_optout branch from 54a6305 to 8763cff Compare December 28, 2021 16:37
@jlestavel
Copy link
Contributor Author

It looks good. Could you rebase?

Done ✔️

@ghiculescu
Copy link
Member

Sorry, could you also squash to a single commit? (https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#squashing-commits)

@jlestavel jlestavel force-pushed the activestorage_assets_precompile_optout branch from 8763cff to 19a7037 Compare December 28, 2021 17:01
@jlestavel
Copy link
Contributor Author

Sorry, could you also squash to a single commit?

Done ✔️

@ghiculescu ghiculescu added the ready PRs ready to merge label Dec 28, 2021
@jlestavel jlestavel force-pushed the activestorage_assets_precompile_optout branch from 19a7037 to dd4f635 Compare January 12, 2022 22:45
@jlestavel
Copy link
Contributor Author

@ghiculescu is there anything missing to approve this PR ?

@ghiculescu
Copy link
Member

It just needs review from the core team :)

@jlestavel
Copy link
Contributor Author

Ok thanks for your answer. No need to add them as reviewers ?

For people who would look for a temporary fix, here's what we added in our config/application.rb to remove the 2 JS assets:

initializer "active_storage.remove_assets", after: "active_storage.asset" do |app|
  app.config.assets.precompile -= %w[activestorage activestorage.esm]
end

Rails.application.config.assets.precompile += %w( activestorage activestorage.esm )
config.after_initialize do |app|
if Rails.application.config.respond_to?(:assets) && app.config.active_storage.precompile_assets
Rails.application.config.assets.precompile += %w( activestorage activestorage.esm )
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just refer to app.config here, rather than the two Rails.application.config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️ right, that's done.

I wrote it like this to be consistent with what you did in 3fd4185 but it's better like that. For consistency again, I changed the code in Action Cable too.

@jlestavel jlestavel force-pushed the activestorage_assets_precompile_optout branch from dd4f635 to 1a46c4a Compare January 18, 2022 12:41
@jlestavel jlestavel requested a review from dhh January 18, 2022 12:43
@dhh
Copy link
Member

dhh commented Jan 18, 2022

Looking good. I know it's a little awkward with tests for this. So just want to confirm that you've verified this working in your local app?

@jlestavel
Copy link
Contributor Author

Yes, I tested it locally using

gem 'rails', '~> 7.0', github: 'wecasa/rails', branch: 'activestorage_assets_precompile_optout'

in my Gemfile. And I could be able to use this new config.active_storage.precompile_assets option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants