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

Add support for Hotwire Turbo Drive #498

merged 4 commits into from Jun 16, 2021


Copy link

@ceritium ceritium commented Jun 11, 2021

It is related to this issue #482

I moved to the gem the @mildred's solution.

I tested it on a small demo app, as I show in the following video:

Any recommendation for better QA?

Copy link

Dainii commented Jun 11, 2021

Tested with rails ( and turbo-rails (0.5.9), it seems to work quite nicely.

@ceritium ceritium marked this pull request as ready for review June 11, 2021 20:25
Copy link

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

This looks great. but please remember:

  1. I am not a rack-mini-profiler developer. I do not have merge rights. (feel free to ignore my suggestions)
  2. I am not a ui developer and just looking at the code and suggesting you copy stuff.

lib/html/includes.js Outdated Show resolved Hide resolved
lib/html/includes.js Outdated Show resolved Hide resolved
Copy link

This looks right to me @OsamaSayegh should we merge?

Copy link

Codecov Report

Merging #498 (2826fc6) into master (a93060e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #498   +/-   ##
  Coverage   87.50%   87.50%           
  Files          18       18           
  Lines        1257     1257           
  Hits         1100     1100           
  Misses        157      157           

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 a93060e...2826fc6. Read the comment docs.

Copy link

@OsamaSayegh OsamaSayegh left a comment

Choose a reason for hiding this comment

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

Thank you for this PR @ceritium ❤️ I'd love to merge this, but I have a couple of comments:


Doing the Turbo constant check avoid adding unnecessary event listeners, but it requires that the application exposes Turbo globally:

import { Turbo } from "@hotwired/turbo-rails"
window.Turbo = Turbo

What do you think? What is the best approach? I guess we should mention it in the documentation.

Requiring people to do something like that feels too much magical to me 😅 Instead, I'd prefer if we added a new enable_hotwire_turbo_support config option (false by default) to lib/mini_profiler/config.rb that people who use Hotwire in their application can turn on. When it's turned on, we can use that to signal to MiniProfiler to add Hotwrite's specific bits.

For example, in lib/html/profile_handler.js I'd replace the data-turbo-permanent="true" attribute that you have now with a new placeholder {hotwriteTurboSupport}. The get_profile_script method here:

def get_profile_script(env)
path = "#{env['RACK_MINI_PROFILER_ORIGINAL_SCRIPT_NAME']}#{@config.base_url_path}"
version = MiniProfiler::ASSET_VERSION
if @config.assets_url
url ='rack-mini-profiler.js', version, env)
css_url ='rack-mini-profiler.css', version, env)
url = "#{path}includes.js?v=#{version}" if !url
css_url = "#{path}includes.css?v=#{version}" if !css_url
content_security_policy_nonce = @config.content_security_policy_nonce ||
env["action_dispatch.content_security_policy_nonce"] ||
settings = {
path: path,
url: url,
cssUrl: css_url,
version: version,
verticalPosition: @config.vertical_position,
horizontalPosition: @config.horizontal_position,
showTrivial: @config.show_trivial,
showChildren: @config.show_children,
maxTracesToShow: @config.max_traces_to_show,
showControls: @config.show_controls,
showTotalSqlCount: @config.show_total_sql_count,
authorized: true,
toggleShortcut: @config.toggle_shortcut,
startHidden: @config.start_hidden,
collapseResults: @config.collapse_results,
htmlContainer: @config.html_container,
hiddenCustomFields: @config.snapshot_hidden_custom_fields.join(','),
cspNonce: content_security_policy_nonce,
if current && current.page_struct
settings[:ids] = ids_comma_separated(env)
settings[:currentId] = current.page_struct[:id]
settings[:ids] = []
settings[:currentId] = ""
# TODO : cache this snippet
script ='../html/profile_handler.js', ::File.dirname(__FILE__)))
# replace the variables
settings.each do |k, v|
regex ="\\{#{k.to_s}\\}")
script.gsub!(regex, v.to_s)
current.inject_js = false if current

is what replaces the placeholders in profile_handler.js with the right values. So, I'd add a check for our new config, and if it's true I'd set hotwriteTurboSupport in the settings hash to data-turbo-permanent="true"; otherwise set hotwriteTurboSupport to the empty string.

Finally, instead of checking for the Turbo constant on the client side, we can check for the data-turbo-permanent attribute on MiniProfiler's <script> tag and add our event listeners if the attribute exists.

The advantage of this is it allows to conditionally add the data-turbo-permanent="true" attribute only for the people who use Turbo, and avoid magical hacks like window.Turbo = Turbo.

One last thing, could you run the update_asset_version rake task when you're done? This updates the ASSET_VERSION constant which we use to bust cache when new versions are released.

Let me know if you have any questions!

@ceritium ceritium changed the title Add support for HotWire Turbo Drive Add support for Hotwire Turbo Drive Jun 15, 2021
@ceritium ceritium force-pushed the turbodrive-support branch 2 times, most recently from fa93363 to 08e7512 Compare June 15, 2021 20:39
Copy link
Contributor Author

Thanks for your feedback @OsamaSayegh. I like your approach. I just pushed a commit addressing your feedback and a small test.

Would you add more tests?
Do you have any suggestions about variable naming?
It's ok the place for enable_hotwire_turbo_drive_support on "config.rb" or would you move it to a different place?

Copy link

OsamaSayegh commented Jun 16, 2021

I think it looks great, thank you @ceritium!

Edit: One more thing; could you please document the new config in table the README file?

Copy link
Contributor Author

@OsamaSayegh I added the documentation.

Do you like the commits as they are? Would you squash them? I didn't found any policy about that.

Copy link

Perfect, thank you for all your work on this!

Github has a merge option that squashes everything together which I'm going to use now!

@OsamaSayegh OsamaSayegh merged commit 1f8b391 into MiniProfiler:master Jun 16, 2021
@ceritium ceritium deleted the turbodrive-support branch June 16, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

6 participants