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 assets to be precompiled with Sprockets #456

Merged
merged 4 commits into from Aug 5, 2020
Merged

Conversation

OsamaSayegh
Copy link
Collaborator

Context #60.

This makes it possible to consume Mini Profiler's assets like any other assets in a Rails app. There is a new config option assets_url which is nil by default but can be changed to a lambda that takes 3 arguments: asset_name (i.e. rack-mini-profiler.{js|css}), version (which is the digest/version of MP assets), and env object, and the lambda should return a string that will be used as the src for the asset. To preserve backward compatibility, the lambda is optional and Mini Profiler will fallback to the current behavior (serve assets from /mini-profiler-resources/*) if no lambda is given or it returns a falsey value.

For most Rails app, the following example will work fine:

Rack::MiniProfiler.config.assets_url = -> (name, version, env) do
  ActionController::Base.helpers.asset_url(name)
end

I tested this change in a brand new Rails app and Discourse and in both apps rake assets:precompile generated MP assets in the public/assets directory, and the assets loaded just fine from the /assets route when I loaded a page in my browser.

Thoughts on this @SamSaffron? Is this OK? Anything you would like me to change or test?

@SamSaffron
Copy link
Member

I think I am fine as long as this is 100% opt in, the PR appears to adding stuff to sprockets by default, we should only add it if people opted to sprockets management.

@OsamaSayegh
Copy link
Collaborator Author

Ok, I've changed this so that the engine is only created if the assets_url config is set. If this looks good I'll add documentation to the README file and merge it in.

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2020

Codecov Report

Merging #456 into master will decrease coverage by 0.27%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
- Coverage   88.01%   87.74%   -0.28%     
==========================================
  Files          22       22              
  Lines        1335     1346      +11     
==========================================
+ Hits         1175     1181       +6     
- Misses        160      165       +5     
Impacted Files Coverage Δ
lib/mini_profiler/config.rb 85.00% <40.00%> (-4.10%) ⬇️
lib/mini_profiler/profiler.rb 83.70% <66.66%> (-0.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60fb0d5...0f53df5. Read the comment docs.

@OsamaSayegh
Copy link
Collaborator Author

I'm going to merge this tomorrow unless there are objections @SamSaffron 😸

@SamSaffron
Copy link
Member

looks good to me!

@OsamaSayegh OsamaSayegh merged commit 79ec6c4 into master Aug 5, 2020
@OsamaSayegh OsamaSayegh deleted the sprocket-assets branch August 5, 2020 09:26
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.

None yet

3 participants