Raise MissingAssetError in compute_asset_path instead of each resolver

[rmacklin]

In rails/importmap-rails#42 (comment) I wrote:

I was looking further into this and noticed that, unlike sprockets-rails, in Propshaft's compute_asset_path helper we don't seem to raise if an asset isn't found: rails/propshaft@e07841a/lib/propshaft/helper.rb#L2-L4

If the resolvers return nil (here or here), then so will compute_asset_path. That will make typos result in a tag like <script src=""></script>:

So I'm going to make PR into Propshaft to make such errors more noticeable (here's the PR where sprockets-rails did so: rails/sprockets-rails#375)

Unfortunately, some things came up and I had to put off working on that. However, a week ago Breno opened #16 to ensure the resolvers raise instead of returning nil! While that fixed the issue of typos potentially going unnoticed, I'd like to propose a small refactor in this PR:

We can also handle it by raising directly in Propshaft's compute_asset_path implementation. I believe that approach is preferable for a few reasons. The simplest reason is that it requires less code.

Additionally, because compute_asset_path is the entry point for anyone looking to understand how Propshaft integrations with the ActionView helpers, I think it's helpful to include the exception it raises right there in the method's definition. This is also consistent with sprockets-rails, which raises AssetNotFound directly in its compute_asset_path implementation.

But most importantly, because compute_asset_path is the highest level of Propshaft's integration with Rails (i.e. the layer just before it hands results back to Rails), I believe it's safest to raise the missing asset error there. Consider a Propshaft extension which monkey patches one of the internal Resolver classes, or introduces a new one, in a way that makes it possible for resolver.resolve to return nil again. In that case, having the raise in compute_asset_path would still protect application developers by ensuring that the missing asset error is still surfaced. As a corollary, authors of such an extension wouldn't have to worry about raising MissingAssetError themselves since Propshaft would handle it.

Thus, I think it makes sense to move the raise MissingAssetError calls out of Propshaft's internal Resolver classes. Propshaft users aren't expected to interact directly with instances of those internal classes, so I think it's fine for their resolve methods to return nil, as the if-expressions naturally did prior to 0fd7814.

As alluded to in #15 (comment), I also thought it would be valuable to add an integration test to ensure everything works as expected within an actual rails application. So, I added a basic integration test in 6ad6b24. (The other commits 88a684d~...de2bf34 are just setup for the integration test which I split out since their large diffs distract from the real "meat" of this PR.) This was extracted to #44.

[dhh]

I appreciate gathering this in just one place, but I'm not convinced it's right. Propshaft's core internal classes should imo provide the complete behavior we'd expect. And I think that means raising on missing. That's not a Rails integration point concern.

[rmacklin]

Thanks for taking a look!

The way I was thinking about it was that folks aren't going to be interacting directly with instances of the internal Resolver classes, so it was fine for those to return nil (as the if-expression naturally did prior to 0fd7814). On the other hand, it's important that the integration point (compute_asset_path) raises because that's what prevents the behavior where typos can go unnoticed when folks invoke an ActionView helper (e.g. asset_path, javascript_include_tag, stylesheet_link_tag) and so I was thinking it'd help to make the raise explicit in the compute_asset_path method body.

I was also thinking that because compute_asset_path is the highest level of Propshaft's integration with Rails (i.e. the layer just before it hands results back to Rails) it's safest to raise the missing asset error there: I can picture someone creating a Propshaft extension which monkey patches one of the internal Resolver classes, or introduces a new one, in a way that makes it possible for resolver.resolve to return nil again. In that case, having the raise in compute_asset_path would still protect application developers by ensuring that the missing asset error is still surfaced.

Edit (2021-12-05): I've expanded the commit message / PR description to better reflect the above.

That said, revisiting the assumption that users won't/shouldn't directly invoke resolve has sparked another thought: Would it be useful for the resolvers to provide an interface with both resolve (which doesn't raise) and resolve! (which raises)? That convention of offering a pair of raising and not-raising methods is familiar to Rails developers and provides flexibility.

[rmacklin]

I rebased to resolve the conflicts.

@dhh Let me know if you have thoughts on the above/how you'd like to proceed on this PR.

[rmacklin]

Tests are failing on this branch after I rebased to resolve another Gemfile.lock conflict, but it seems like a legitimate issue in the main branch. I've filed #41 with more details (along with a PR to fix it: #42).

That said, I think this issue demonstrates the value in having a test app and real integration tests. The offending commit passed CI because the existing test suite isn't exercising that initializer in the railtie.

[dhh]

I think having integration tests is a good deal. But let's slim the dummy way down to just what we need.

[rmacklin]

I regenerated the test app with --skip-active-record --skip-active-storage --skip-action-mailer --skip-action-cable and then removed a bunch of unnecessary files, so it's much slimmer now:
https://github.com/rails/propshaft/compare/e1962557a8766e15f36e094585ba3e2b4ef315a7..f75c851410a90399e7c75900ee65ca05468b0d7a

[rmacklin]

Side note: I can repeat my steps to slim down the test app in importmap-rails if we want to do that, too.

[rmacklin]

I've updated the PR after the integration test was extracted to #44. Still interested in thoughts on #20 (comment) as well as a new commit I added: 19dce11