Remove auto-adding after jquery.js script — Sprockets 4 (nor webpack-based javascript_pack_tag 'application') doesn't add a separate script tag for jquery

TylerRick commented 3 years ago

In both a real app that I just upgraded to sprockets 4 and the spec/dummy app in this project, I noticed that javascript_include_tag "application" no longer injects a separate script tag for jquery.

With Sprockets 3, this line:

//= require jquery

in spec/dummy/app/assets/javascripts/application.js used to result in separate script tags being emitted, like this:

  <link rel="stylesheet" media="all" href="/assets/application.self-6a96ae63b4989f39d1d63e39f5a0503899fed3893103a892c5d1fb7ec414c771.css?body=1" />
  <script src="/assets/jquery.self-bd7ddd393353a8d2480a622e80342adf488fb6006d667e8b42e4c0073393abee.js?body=1" nonce="GaKc6o31RgAGbcMmMg3IFg=="></script>
<script src="/assets/xray.self-130d130a29b059ab5e1f6b7ea349011b67ee8593ecb3e9222db86c7a5d55ed3a.js?body=1" nonce="GaKc6o31RgAGbcMmMg3IFg=="></script>
<script src="/assets/jquery_ujs.self-784a997f6726036b1993eb2217c9cb558e1cbb801c6da88105588c56f13b466a.js?body=1" nonce="GaKc6o31RgAGbcMmMg3IFg=="></script>
<script src="/assets/application.self-439a617686e9aaa7030e6077dbcb8865f923f7e2641960195469b371a3e62143.js?body=1" nonce="GaKc6o31RgAGbcMmMg3IFg=="></script>

  <meta name="csrf-param" content="authenticity_token" />

But now with Sprockets 4, it concatenates everything and just emits this instead:

<head>
  <title>Xray</title>
  <link rel="stylesheet" media="all" href="/assets/application.debug-c3878a14baa1490b0ff19b1a76d4f2191b9d22ae3f0a54a3e14bc36ce2a7766e.css" />
  <script src="/assets/application.debug-066afcd8a4e2870db44250c44962560df36dc70e1ad58357e44a8676d87a3bf3.js" nonce="LZpwX8uFDMkts/WHAcWRWA=="></script>
  <meta name="csrf-param" content="authenticity_token" />

(BTW, I've already adjusted the script matching to account for new .debug suffix, in #101)

That's fine. Concatenating them into a single script is what it should be doing (at least in production). I still don't understand what was making it emit the separate jquery.js line before — or why xray-rails is designed to depends on that line being there to even work properly.

Maybe the Rails.application.config.assets.debug is what caused it to split that into sub-assets in dev before? But that is still set to true. So maybe sprockets/rails changed that behavior.

Workaround

Whatever the case, as a workaround for now, this is what I did in my real app:

= javascript_include_tag 'xray', nonce: true if Rails.env.development?

I just included the xray script directly, because I know jquery is included in my application.js script, and since xray-rails no longer adds that for me.

Inside this project, I worked around it slightly differently (in #101):

  <%= javascript_include_tag "jquery", nonce: true  if Gem.loaded_specs['sprockets'].version >= Gem::Version.new('4.0.0') %>

... because the tests are still testing to make sure that xray.js gets added automatically, and if I just included

  javascript_include_tag 'xray'

then it would no longer be testing that xray.js gets added automatically (which might be okay — maybe we should just remove that feature).

What's a better solution?

Personally, I'm starting to think that the feature where it automatically adds xray.js to the document is more trouble than it's worth.

I propose that we remove that automatic feature and just make it part of the install instructions that you need to add this line to your template/layout:

<%= javascript_include_tag 'xray', nonce: true if Rails.env.development? %>

Sure, zero-config one-click installs are nice and all, but:

It makes xray-rails more complex than it needs to be — if I had just done it that way from the start, I could have completely avoided doing all the work I did for #101 :cry:
It requires making too many assumptions — xray can't know (and shouldn't care) how your app requires jQuery. It should just require that the developer has provided it somehow.
- It also doesn't know whether the loading of the jquery script is deferred (see #98)

I guess that's why this branch exists — to give you that flexibility:

          if body =~ script_matcher('xray')
            # Inject the xray bar if xray.js is already on the page
            inject_xray_bar!(body)

I guess I'm just wondering why we should even bother keeping the other (add xray.js after jquery.js) branch. Is it really worth it?

It might have been a fair enough assumption back in the olden days (2012), but these days (with use of webpack in Rails projects the norm, etc.), it doesn't make as much sense. For that matter, why not remove the dependency on jQuery entirely (#111)?

What about projects using webpack for JS instead of sprockets?

We should also think about what the right way to include xray into a project if it is using webpack for JS (which I understand is the default way in Rails 6, though I have not upgraded yet in my apps) instead of sprockets...

In that case, we will have the same problem: the template will have a line like this:

javascript_pack_tag 'application'

which will not add a separate script tag for jquery.js either.

(See also #106)

rocket-turtle commented 3 years ago

I would love to see this and this gem move forward.

If the auto adding is removed completly the gem would be a lot easier. It is a nice feature but tends to break and adds a lot of comlexity. If the user adds the JavaScript and the xray_bar then the methods like script_matcher append_js! can go away.

Maybe there could be a Verision 1.0:

No auto include
No jQuery required
Ruby 2.5+ (including ruby 3.0)
Rails 5.2+
Sprockets 3 and 4
Webpacker (I do not use it but I think this should work without the auto include)

TylerRick commented 1 year ago

I like your idea, @rocket-turtle . Sad to see this project languish...

brentd / xray-rails