Decouple from sprockets-rails

[rmacklin]

As noted in #42, the gem had a direct reference to a constant from sprockets-rails here:

importmap-rails/lib/importmap/map.rb

Line 94 in 8d66a09

rescue Sprockets::Rails::Helper::AssetNotFound

This is problematic because consumers may be using a different asset pipeline than sprockets-rails. In that case, when Importmap::Map#resolve_asset_paths invokes the application's asset_path helper method, it may need to rescue different exceptions than Sprockets::Rails::Helper::AssetNotFound.

To support that, we've added a rescuable_asset_errors property to the importmap configuration options. We automatically add Sprockets::Rails::Helper::AssetNotFound to the list if Sprockets::Rails is defined, which should allow the gem to work the same as before in applications that use sprockets-rails. But now it'll also be usable in applications that use something other than sprockets-rails.

[rafaelfranca]

Maybe a better fix would just to rescue all exception and add the exception name, message, and backtrace to the warning.

[rafaelfranca]

This only works if import-maps is loaded before sprockets-rails.

[rmacklin]

Did you mean after sprockets-rails? I think if importmap-rails was loaded before sprockets-rails this would not work, i.e. defined?(Sprockets::Rails) would be false even though it's going to be defined later once sprockets-rails gets required.

So there does seem to be a load order dependency here. In the past, I've always worked on applications where sprockets/railtie was required before Bundler.require(*Rails.groups) was invoked, because that's how rails has generated it...but that won't be true in rails 7 after rails/rails#43261. I think we can make it work independent of the load order by inserting into this list within an initializer block.

[rmacklin]

Addressed in https://github.com/rails/importmap-rails/compare/34defe58e2cdfa4fe52d03821083da1d0fd57e68..313e2cf3407b83ca6c19f138389f6640efeb6cb3

[rmacklin]

Maybe a better fix would just to rescue all exception and add the exception name, message, and backtrace to the warning.

I'm not against that, but is there any concern that it would swallow too many exceptions? Currently this PR effectively preserves the existing behavior, whereas rescuing everything would be a broader change.

[rafaelfranca]

I'd more concerned if this rescue was around more code. But it is only around asset_path. if it failed because sprockets could not resolve, or because there is a bug in sprockets I don't think it would be a problem if we show the exception class, message and backtrace in the warning message.

[rafaelfranca]

Current fix is fine though, so I think it is fine as is.

[rromanchuk]

Hey y'all this is embarrassing how long it took me to figure out, but it wasn't until i noticed my Gemfile.lock had no mention of sprocket-rails. I just assumed /bin/rails importmap:install would have already handled that since it's already generating manifest templates etc.

It was surprisingly tricky to debug because everything was working as expecting with a quiet browser console and correct html injection so figured it was user config error.

It wasn't until i noticed

ActionController::RoutingError (No route matches [GET] "/javascripts/es-module-shims.min.js"):
ActionController::RoutingError (No route matches [GET] "/turbo.min.js"):

This might be somewhat common for mature projects who were webpack only, but now coming back to asset pipeline

[rromanchuk]

Instantly working, you have no idea how far into the weeds i got 🤣 🤦

devlog        | Started GET "/assets/es-module-shims.min-064d3375656d4550e01a95cfeb330fb651e5e053fd3a694691aa131870e7f307.js" for 127.0.0.1 at 2021-12-28 18:50:50 -0700
devlog        | Started GET "/assets/application-c1762107b60220a6e963367c3db9dfb8aee8144d6bd5c3ce0a0036565cbe70b6.js" for 127.0.0.1 at 2021-12-28 18:50:50 -0700
devlog        | Started GET "/assets/turbo.min-2e35750e215200b3e20412a6fa49c166604997c83c17a063885c0e06c5c5c0fe.js" for 127.0.0.1 at 2021-12-28 18:50:50 -0700

[rmacklin]

@rromanchuk, I'm not sure if you were suggesting that the importmap:install task should install sprockets-rails or just stating that you had assumed it would. To clarify, importmap-rails is meant to work with alternative asset pipeline gems, such as propshaft, so it wouldn't make sense for the importmap:install task to install sprockets-rails on your behalf.

Also note that sprockets-rails was a dependency of rails prior to version 7.0, so the only time you'd run into that issue is if you were upgrading to rails 7. And in that case, it's highly recommended to read the upgrade guide, which mentions that you'll need to add sprockets-rails to your Gemfile if your application needs it (i.e. if you aren't using an alternative asset pipeline):
https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#sprockets-is-now-an-optional-dependency

[rromanchuk]

were upgrading to rails 7. And in that case, it's highly recommended to read the upgrade guide

I did, but i don't (didn't) use sprockets so it's NA information. It wasn't until i read dhh et al posts that talked about pipelines, let alone extensible ones. There isn't any mention of sprockets https://github.com/rails/importmap-rails#installation and running in development obfuscates the until obvious becomes true.

https://github.com/rails/importmap-rails

All you need is the asset pipeline that's already included in Rails

I'm using this because i never want to think about transpiling and precompiling again. Since there is no opinionated default pipeline, in order to run this end to end, the user is required to set one up. The upgrade guide says sprockets is now optional, and this readme makes zero mentions of needing to bring your own dependency. Just a mention in the docs would suffice.