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

Restore :assets group in Gemfile #250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tom93
Copy link
Contributor

@tom93 tom93 commented Jan 5, 2024

Some of those gems (especially mini_racer) are very large, and it is useful to be able to skip them. They were already in a commented-out group; restore the group to allow skipping them (they will still be installed by default).

Note that putting them in a group will mean they won't be auto-required when config/application.rb calls
Bundler.require(:default, Rails.env), but that should be fine.

Also remove the old comment "not required in production environments by default" because it is unclear/inaccurate (we currently run the asset precompilation on the production server), and add hints on how to skip the group.

Some of those gems (especially mini_racer) are very large, and it is
useful to be able to skip them. They were already in a commented-out
group; restore the group to allow skipping them (they will still be
installed by default).

Note that putting them in a group will mean they won't be
auto-required when config/application.rb calls
`Bundler.require(:default, Rails.env)`, but that should be fine.

Also remove the old comment "not required in production environments
by default" because it is unclear/inaccurate (we currently run the
asset precompilation on the production server), and add hints on how
to skip the group.
@coveralls
Copy link

Coverage Status

coverage: 37.131%. remained the same
when pulling 07ea3cd on gemfile-assets-group
into b26315e on master.

@Holmes98 Holmes98 self-requested a review January 5, 2024 23:20
Copy link
Member

@Holmes98 Holmes98 left a comment

Choose a reason for hiding this comment

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

I'm not quite sure I fully understand why, but the assets group was removed in Rails 4:

And this doesn't seem ideal, but we might be compiling some assets live in production mode; not sure if that could cause issues with this:

# Don't fallback to assets pipeline
# config.assets.compile = false

@tom93
Copy link
Contributor Author

tom93 commented Jan 6, 2024

Good catch, what a mess. Just wanted to skip some gems...
I'm tempted to keep the assets group (even though Rails got rid of it), because that seems like the way to allow those gems to be skipped.
I'll definitely first double check that asset precompilation still works identically (some of those comments about needing to patch the rake task and missing initializers got me worried).

we might be compiling some assets live in production mode

I hope that can be dealt with separately? (The gems will still be installed in production, just won't be auto-required by default; I'll try to confirm that live compilation still works with that change.)

@Holmes98 Holmes98 dismissed their stale review January 6, 2024 08:02

Feel free to merge this if you don't find any issues.

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

Successfully merging this pull request may close these issues.

4 participants