8000 add default_format method to RailsTemplate for Turbo compatibility by lazylester · Pull Request #1144 · haml/haml · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add default_format method to RailsTemplate for Turbo compatibility #1144

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 27, 2023
Merged

add default_format method to RailsTemplate for Turbo compatibility #1144

merged 1 commit into from
Sep 27, 2023

Conversation

lazylester
Copy link
Contributor

as discussed in 'issues'

@lazylester lazylester mentioned this pull request Sep 15, 2023
Closed
@k0kubun
Copy link
Member
k0kubun commented Sep 27, 2023

Thanks!

@k0kubun k0kubun merged commit fb629e9 into haml:main Sep 27, 2023
timdiggins added a commit to timdiggins/haml-issue-1154 that referenced this pull request Oct 10, 2023
@timdiggins
monkey-patch to workaround haml/haml#1154
8162a1c 
basically revert haml/haml#1144
@timdiggins
Copy link
Contributor

I think this change should be rolled back for the following reasons:

  • Not all uses of Haml are to create html content-type responses and this can cause issues for them (e.g. js or js with inline html).

  • It seems like dhh's comments on the original issue in turbo-rails just before he closed it (Wrong Content-Type with HAML hotwired/turbo-rails#287 (comment)) were to say that the naming convention (including .html. in view template names) is required.

  • I don't think Haml should have code that is there to just make Turbo-rails work for people who don't want to follow this convention. And it's pretty easy to rename all your xxx.haml files to xxx.html.haml files - or to add a monkey patch in your app.

NB: "default_format" is quite underdocumented and seems confusingly named - when you define it you prevent the actual default fallback format (which is empty) from being used ( see ActionView::TemplateDetails::Requested.format_or_default)

@lazylester
Copy link
Contributor Author
lazylester commented Oct 10, 2023 •
edited
Loading

@timdiggins haml has historically always supported #default_format. If it's considered appropriate to remove this support, as happened when hamlit was merged, then it should first be deprecated, so as to smooth the path for developers who are upgrading.

If restoring support for '#content_type` is causing problems in some scenarios (I'm not at all clear on the js scenario you mention), then it must always have been problematic, surely?

Whilst DHH may 8000 assert that inclusion of .html in view templates is required, it actually never has been a requirement, but simply common practice.

@k0kubun
Copy link
Member
k0kubun commented Oct 10, 2023

Can either of you work on a pull request? As long as it passes tests, I'm open to merging one.

@timdiggins timdiggins mentioned this pull request Oct 11, 2023
Closed
@timdiggins
Copy link
Contributor

Long response - hope it's helpful.

@k0kubun I'm not sure what the pull request would be - I'm proposing reverting this change, but there are diverse points of view, so I guess we need to agree what the solution should be before working on code.

@lazylester I wasn't aware of the previous use of default_format in haml (pre haml 5 I'm guessing, but I haven't found a def default_format in v3 or v4). In any case the re-use of default_format for template resolution purposes was effectively only started in rails 6 I believe (with this commit: rails/rails#35404) so those earlier uses wouldn't have impacted anyone. [3]

And my point about this not being appropriate in haml, as it's only rails-related, is probably irrelevant as haml already contains a lot of rails-targeted code.

Here's my understanding of the situation. The work that default_format does is to change the way that templates are resolved (discovered by their name in the filesystem), by rails. Confusingly, the (response) format is determined by rails based on the request, and is not (AFAICT) adjusted by the "default_format" that the templating system defines (the work of default_format in the templating system does not seem to be documented anywhere in rails sadly , despite it being used by many templating plugins like haml). This response format could be any defined mime-type's symbol (html, js, json, csv... and/or a custom defined one).

The rails template resolver looks for a template matching the action.format.handler where handler is a Template Handler's extension (haml, erb, etc). Layout and partials are looked up using the same mechanism. (There's actually locales and variants as well, but I'm simplifying, see https://github.com/rails/rails/blob/8776e951f269dc161074aa75f76f115289e14c3a/actionview/lib/action_view/template/resolver.rb#L26-L36).

For partials and layouts, if the exact match doesn't exist, that's where default_format kicks in. Without a default_format, the resolver looks for (will match on) layout_or_partial.handler (a nil format in other words). But if the template defines a default_format then the resolver looks for (will match on) layout_or_partial.default_format.handler.

So haml defines their default_format as html, and the user of haml follows rails conventions, then they will have a surprise - a js format which uses haml will select the application.html.haml layout, not application.haml (or more likely, if they haven't defined any application.handler layout, skip the layout[1])

The answer is for everyone using formats which are not html with haml, to make sure to define layout application.js.handler (for example[2]) as empty, or say layout: false, and for partials to make sure there's an exact match. And if multiple non-html formats require exactly the same content (e.g. the edge case outlined in #1154) then you have to duplicate the files with each format.

It could be that the examples I've given here are edge cases and not important. But the options are:

  • "(re)name your files with html.haml" (for haml to work well out of the box with Turbo-rails) or
  • "reduce the functionality and break some people's existing code for existing haml users"

But i think all of this discussion is mostly about rails and how it behaves. Hence my feeling that this code belongs in rails or (as a monkey patch) in an app.

[1] the rule for layouts is -- no match, then skip the layout, whereas for partials, it's no match, then error.
[2] when I say handler here, I mean any of erb, haml, etc.
[3] The actual use for template resolution is (as of rails 7.1) in https://github.com/rails/rails/blob/8776e951f269dc161074aa75f76f115289e14c3a/actionview/lib/action_view/template_details.rb#L63 and https://github.com/rails/rails/blob/8776e951f269dc161074aa75f76f115289e14c3a/actionview/lib/action_view/unbound_template.rb#L55.

@lazylester
Copy link
Contributor Author

good research @timdiggins.

I think there's a third option, going forwards, which is to deprecate the use of #default_format and announce the intention to remove it in a later version. Really this has been my point all along, that behaviour shouldn't just be dropped silently.

@timdiggins
Copy link
Contributor

@lazylester unless I misunderstood, your third option seems to refer to how rails should work, not how haml works? Using the template default_format is a novelty in Haml 5 / 6, so "deprecating it" doesn't make sense I think (why introduce something in a patch release and then deprecate it).

I think the monkey patch approach would still work for people using Turbo-Rails (just only with RailsTemplate and without the Plugin pathc) and wouldn't cause any issues with existing Haml users who aren't using turbo (or not yet, or not exclusively).

Hence why I'm suggesting reverting this. Thoughts Les, or have I got the wrong end of the stick?

@lazylester
Copy link
Contributor Author

The monkey patch works just fine. The problem I'm addressing, and the one I experienced, is that my app broke when I upgraded to the hamlit version of haml, because a feature of haml had been dropped. So I had to spend some time troubleshooting the internals of rails turbo to figure out why. Since it appears to me to be a pretty benign feature to restore, it seemed like an easy way to reduce the migration pain for other developers.

All your forensic work is interesting, but it doesn't address the problem that I, and evidently others too (from the rails issue discussion) have encountered.

@timdiggins
Copy link
Contributor

All your forensic work is interesting, but it doesn't address the problem that I, and evidently others too (from the rails issue discussion) have encountered.

Ah, ok, yes, I wasn't very clear! My suggestion is to revert this commit, but also as part of that PR noting the requirement for turbo-rails compatibility in either the documentation or in the CHANGLEOG for Haml 6 breaking changes something like the following:

For people using Turbo-rails and Haml 6+ need to either:

  • follow the naming convention with the html format (ie. ensure any .haml files end .html.haml), or

  • add a monkey patch as follows:

     # config/initializers/haml.rb
 require "haml/rails_template"

 module Haml
   class RailsTemplate
     def default_format
       :html
     end
   end
 end

@lazylester What do you think? Would you agree to this change? If so, I'm happy to contribute the PR if you don't want to. But I wouldn't want to contribute the reversion PR without your agreement.

@lazylester
Copy link
Contributor Author

documenting the problem is good, but not as good as a deprecation notice. But I'm still not understanding why inclusion of the default_format method is problematic. A js file with inline html, you said. Wasn't this problematic for haml prior to its merging with hamlit, when default_format was supported? Is there some framework other than turbo that is testing the value of default_format and getting the wrong result?

@k0kubun
Copy link
Member
k0kubun commented Dec 10, 2023

Thank you both, @timdiggins and @lazylester, for your thoughtful discussion on this issue.

haml has historically always supported #default_format

I wasn't aware of the previous use of default_format in haml (pre haml 5 I'm guessing, but I haven't found a def default_format in v3 or v4)

Using the template default_format is a novelty in Haml 5 / 6

Wasn't this problematic for haml prior to its merging with hamlit, when default_format was supported?

Let me get this straight. Here's the template_handler registered to Rails in v4 (no influence of Hamlit, which should have accumulated all historical code) and v5 (not entirely replaced by Hamlit yet, but migrated to Temple):

Neither of these defines #default_format, at least inside Haml. So, to my knowledge, haml.gem defining #default_format is completely a new feature in v6.1.3.

Since this is a fairly new feature, I believe reverting it is also fixes #1152 and #1154, and this was not consistent with the recommendation by DHH himself, I'll revert this as @timdiggins proposed, in a minor release as a semi-breaking bugfix.

To mitigate the impact for Turbo users, I'll cut a teeny release before that which only prints a deprecation warning in #default_format as @lazylester proposed, and commit #1144 (comment) as a documentation.

k0kubun added a commit that referenced this pull request Dec 10, 2023
@k0kubun
Warn the use of #default_format
9357721 
ref: #1144 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
4BAD
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lazylester @k0kubun @timdiggins
0