k0kubun / hamlit

High Performance Haml Implementation
https://rubygems.org/gems/hamlit
Other
980 stars 60 forks source link

Haml gem dependency has impact on app behaviour #184

Closed igor-drozdov closed 3 years ago

igor-drozdov commented 3 years ago

Hi @k0kubun 👋

I've decided to revisit the discussion we had in https://github.com/k0kubun/hamlit/pull/178 and create a sample app that reproduces the problem I was talking about: https://github.com/igor-drozdov/hamlit_app/commits/main.

It's a simple rails app that has hamlit installed. Also haml_lint is specified in the Gemfile (it can be any other gem that has haml as a dependency, for example html2haml).

A simple spec expects haml to be rendered:

require 'rails_helper'

RSpec.describe "greetings/hello.html.haml", type: :view do
  it 'expect class names to be with double quotes' do
    render

    expect(rendered).to eq(%{<h1 class="hello">Greetings#hello</h1>\n})
  end
end

The last 4 commits reproduces the issue.

1) When we're on the Generate a view (a389f9c8d2d4a82edbde319a01ceef3dd230ae1c) commit, the spec fails because double quotes are expected.

2) When we're on the Set Hamlit's attr_quote to double quote (db160d9ea05949f45f4e7353cab157f01674571d) commit, the spec fails even though we've set the attr_quote option to double quote

3) When we're on the Set Haml attr_wrapper option to double quote (f9d0cecf0cfcc825d004e9352ff7bf463401f466) commit, the spec passes just because we've set the Haml's attr_wrapper to double quote instead

4) When we're on the Remove haml_lint gem (1347d7a081a7a6beff869cc224e38b4718c8e3ff) commit, the spec passes because we've set attr_quote option to double quote and removed the haml_lint gem

The 1, 4 steps are expected, but the fact that some other gem dependency existence or option setting has impact on the hamlit behavior is a bit confusing. Maybe we can indeed introduce an option as you've suggested before, since the require 'haml' code is mostly for Sinatra apps 🤔 Just let me know, what you think, thanks!

k0kubun commented 3 years ago

Hi, thanks for creating the reproductive repository. That was really helpful.

I think the "right" fix would be to create haml-parser.gem and let haml_lint.gem rely on it instead of depending on the entire haml.gem. However, haml_lint.gem is out of my control, and carving out haml-parser.gem is a bit of work, so I decided to make a workaround this time.

I chose an option that requires no extra config and introduced the fix in v2.15.0. I confirmed that it passes the spec with haml_lint.gem in your repository, but please confirm that it works in your application too.

igor-drozdov commented 3 years ago

@k0kubun Wow, you're amazing! Thank you for such a quick turnaround! 🙇

I can confirm that the issue I've described is fixed 🎉