-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
does not work with rails 7 #59
Comments
I researched Rails source code for a while and found OptimizedFileSystemResolver has totally removed. It maybe replaced with https://github.com/rails/rails/blob/2daef106afc37f385867a5543b06e7b8f2fb908e/actionview/lib/action_view/template/resolver.rb and nodoc at all. Rails upgrade guide doesn't mention it as well cause it's an inner refactor. |
Thanks for the issue. Do you have a small example app that reproduces the issue? https://codetriage.com/example_app |
Thanks for the response. After making the test app with a clean Rails 7 app, there was no issue. However, once I added the "render_async" gem, the error starts to get thrown, claiming to be a maildown issue. https://github.com/maxwell/maildown-example I'll look into render_async to see why that might be causing the issue. |
I'm not quite sure what might be the issue here, but the https://github.com/renderedtext/render_async/blob/master/lib/render_async.rb Is it possible it is some sort of sad load order issue, combined with the monkeypatch from maildown? |
@maxwell I get a similar issue. On a clean rails 7 app (rails new) there are no issues, however when there are other gems present (including render_async as per your sample app), it errors out pointing at maildown. |
@maxwell No. I don't use gem |
I created a sample app reproducing the issue here. I did not have to add any gems beyond As @raykin pointed out, The sample repo includes a patch that's working well for me. The
I'm guessing this means the tests are dependent upon whatever version of Rails happens to be installed. I'm not really sure how best to make this work with multiple versions of Rails. |
@adamlogic that patch is also working for me. Do you feel it's good enough to submit a PR? |
I'm not sure. Making this gem work with multiple versions of Rails is complicated, and I haven't thought through the implications of my patch beyond my own Rails 7 app. |
If you want to turn this into a patch that fixes Rails 7 then I can do the leg work of getting older versions to work on the gem. Im also wishing/wondering if maybe there’s a more bulletproof way to get what we want. Like maybe we hook into sprockets and generate the html/plaintext assets at deploy time (or something). The way this gem works is fundamentally opposed to a core concept with Rails (that mime types and outputs are 1:1 correlated with files on disk). We are able to hack it via monkey patches but ideally for this to be sustainable into the future we either need to do something different or get Rails to provide a public “blessed” way to hook into its rendering pipeline to generate two different outputs from one input file. let’s get rails 7 working for now and then maybe revisit longer term maintainability. |
@adamlogic I tried your patch, it did work in development but at last failed on production when deploying to Heroku. The crash raised before app start and said
After hours of debugging, I get a new solution to just satisfy OptimizedFileSystemResolver in maildown. module ActionView
# No required for Rails7
class OptimizedFileSystemResolver
def extract_handler_and_format_and_variant(template)
# just set it for outdated maildown
end
end
end @schneems This gem actually works well with Rails7 without OptimizedFileSystemResolver. I also add a new case in my project to verify it works. ## prepare email
assert_equal 2, email.parts.size
assert_equal "multipart/mixed", email.mime_type
assert_equal "text/plain", email.text_part.mime_type
assert_match er.content, email.text_part.body.to_s
assert_equal "text/html", email.html_part.mime_type Just a little change, multipart/alternative becomes multipart/mixed |
Documented in #59. This class was removed from Rails 7. This causes an error on this line when trying to alias a method that does not exist https://github.com/zombocom/maildown/blob/cbf5b51e0867eab1ec945d38b5335f42139b0322/lib/maildown/ext/action_view.rb#L7. The solution presented was to re-add the method. However simply doing this with a `defined?(OptimizedFileSystemResolver)` causes a load via zeitwork and also results in a crash. The presence of this code appears to do nothing in Rails 7. It's basically a no-op that allows us to boot without erroring.
Documented in #59. This class was removed from Rails 7. This causes an error on this line when trying to alias a method that does not exist https://github.com/zombocom/maildown/blob/cbf5b51e0867eab1ec945d38b5335f42139b0322/lib/maildown/ext/action_view.rb#L7. The solution presented was to re-add the method. However simply doing this with a `defined?(OptimizedFileSystemResolver)` causes a load via zeitwork and also results in a crash. The presence of this code appears to do nothing in Rails 7. It's basically a no-op that allows us to boot without erroring.
Documented in #59. This class was removed from Rails 7. This causes an error on this line when trying to alias a method that does not exist https://github.com/zombocom/maildown/blob/cbf5b51e0867eab1ec945d38b5335f42139b0322/lib/maildown/ext/action_view.rb#L7. The solution presented was to re-add the method. However simply doing this with a `defined?(OptimizedFileSystemResolver)` causes a load via zeitwork and also results in a crash. The presence of this code appears to do nothing in Rails 7. It's basically a no-op that allows us to boot without erroring.
Documented in #59. This class was removed from Rails 7. This causes an error on this line when trying to alias a method that does not exist https://github.com/zombocom/maildown/blob/cbf5b51e0867eab1ec945d38b5335f42139b0322/lib/maildown/ext/action_view.rb#L7. The solution presented was to re-add the method. However simply doing this with a `defined?(OptimizedFileSystemResolver)` causes a load via zeitwork and also results in a crash. The presence of this code appears to do nothing in Rails 7. It's basically a no-op that allows us to boot without erroring.
* Update maintenance policy and dummy gemfiles * Update check changelog * First Github CI commit * Fix missing net/smtp error ``` Error: LayoutsTest#test_no_layout: LoadError: cannot load such file -- net/smtp activesupport (6.1.6) lib/active_support/dependencies.rb:332:in `require' activesupport (6.1.6) lib/active_support/dependencies.rb:332:in `block in require' activesupport (6.1.6) lib/active_support/dependencies.rb:299:in `load_dependency' activesupport (6.1.6) lib/active_support/dependencies.rb:332:in `require' mail (2.7.1) lib/mail.rb:9:in `<module:Mail>' ``` https://stackoverflow.com/questions/70500220/rails-7-ruby-3-1-loaderror-cannot-load-such-file-net-smtp * Patch for Rails 7 Documented in #59. This class was removed from Rails 7. This causes an error on this line when trying to alias a method that does not exist https://github.com/zombocom/maildown/blob/cbf5b51e0867eab1ec945d38b5335f42139b0322/lib/maildown/ext/action_view.rb#L7. The solution presented was to re-add the method. However simply doing this with a `defined?(OptimizedFileSystemResolver)` causes a load via zeitwork and also results in a crash. The presence of this code appears to do nothing in Rails 7. It's basically a no-op that allows us to boot without erroring.
Also, thanks a ton for the reproduction and the discussion. Once I finally got Rails 7 tests working on GitHub actions it made it much easier to patch. |
The text was updated successfully, but these errors were encountered: