Don't compile external templates - #94
Conversation
|
@marcoroth im seeing some test fails here, but I think they're on main. Anything I can do to help? |
|
@jherdman thanks! I think you might have to run |
| class_attribute :erb_implementation, default: Handlers::Herb::Herb | ||
|
|
||
| def call(template, source) | ||
| return compile_with_fallback_erb_implementation(template, source) unless local_template?(template) |
There was a problem hiding this comment.
If people opt-in to use Herb (or have a .herb file) we should compile everything using Herb. So I think we want to move this condition to lib/reactionview/template/handlers/erb.rb:10:
There was a problem hiding this comment.
OK, I think I understand your motivation, but I want to double check. Am I correct to understand that ReActionView::Template::Handlers::Herb is never used unless we intercept ERb files?
There was a problem hiding this comment.
Correct yes, see here:
reactionview/lib/reactionview/railtie.rb
Lines 28 to 34 in 416995f
So if you have .html.herb it uses ReActionView::Template::Handlers::Herb directly, and otherwise if you have intercept_erb = true it goes through ReActionView::Template::Handlers::ERB
There was a problem hiding this comment.
Ideally, we would still try to compile everything through Herb::Engine and only fall back (and warn) if there are some parse errors. But we don't need to do this in this PR.
There was a problem hiding this comment.
Do you reckon the cost of a rescue, log, retry through the fallback, is acceptable? Implementation doesn't seem that difficult imho.
There was a problem hiding this comment.
Since this is a compile-time concern I'm not too worried about the performance cost, that said, we could also just do it in development/test and not in production.
User's should not be beholden to template errors outside of their application. When we detect a template not local to the project we use the fallback ERB implementation.
5b7269c to
05ead32
Compare
|
@marcoroth just a friendly ping to see if there's anything else to be done here. |
|
Hey @jherdman, I have been travelling and working on a new talk for Tropical on Rails this week. I'm getting back to ReActionView after Tropical on Rails and RubyKaigi is over. Thank you! 🙏🏼 |
|
Hey gang. Sorry to poke again, but is there anything I can do to help this along? |
Follow up on #94. This pull request adds `config.external_template_mode` to control what happens to templates that come from gems rather than from the application itself. #### Motivation With `intercept_erb` enabled, ReActionView sees every `.html.erb` template Rails renders, including ones shipped inside gems. #94 stopped compiling those, which fixed #91, but it did so silently. If a gem's templates cannot be compiled by Herb, you never find out. That silence is the objection @joelhawksley raises in marcoroth/herb#1508, where a vendored copy of `primer/view_components` contains ERB that Herb cannot compile: So rather than a binary "skip or don't", the mode says how loudly to handle the failure: | Mode | Behavior | | --- | --- | | `:fallback` (default) | Compile with Herb. If that fails, log a warning and fall back to `ActionView::Template::Handlers::ERB`, so the template renders exactly as it would without ReActionView installed. | | `:skip` | Never compile templates that come from gems. This is what #94 shipped. | | `:compile` | No special treatment. Your `validation_mode` applies to them just as it does to your own templates, and nothing is rescued. | ```ruby ReActionView.configure do |config| config.external_template_mode = :fallback end ``` A gem template Herb cannot handle now renders as before, and says so: ``` [ReActionView] /app/vendor/bundle/ruby/3.4.0/gems/actionpack-8.1.2/lib/action_dispatch/middleware/templates/rescues/routing_error.html.erb could not be compiled by Herb, falling back to ActionView::Template::Handlers::ERB: InvalidNestingError: Block element <h2> cannot be nested inside <p> at line 9 ``` Related marcoroth/herb#1508 Related marcoroth/herb#1362 Related #91
3 participants
User's should not be beholden to template errors outside of their application. When we detect a template not local to the project we use the fallback ERB implementation.
Resolves #91