k0kubun / hamlit

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

Remove requiring 'haml' gem #178

Closed igor-drozdov closed 3 years ago

igor-drozdov commented 3 years ago

We have hamlit and haml_lint specified in the Gemfile:

gem 'hamlit', '~> 2.14.4'
gem 'haml_lint', '~> 0.37.0', require: false

Even though, haml_lint is not required, it's installed along with its dependencies:

haml_lint (0.36.0)
  haml (>= 4.0, < 5.3)
  parallel (~> 1.10)
  rainbow
  rubocop (>= 0.50.0)
  sysexits (~> 1.1)

Due to this line, the haml dependency is loaded, its methods are called and even has some impact on the application behavior.

I see that haml is a development dependency.

When I remove:

# Load tilt/haml first to override if available
begin
  require 'haml'
rescue LoadError
else
  require 'tilt/haml'
end

The hamlit tests still pass. Maybe it can be removed, since haml has been removed from Gemfile? https://github.com/k0kubun/hamlit/commit/407e785cf36d586bcba7746bfb91bc0d5f275c9e#diff-d09ea66f8227784ff4393d88a19836f321c915ae10031d16c93d67e6283ab55f

igor-drozdov commented 3 years ago

@k0kubun Thank you for the regular maintenance! The low number of open issues is impressive! I wonder if you remember what problem has to be solved by adding these lines 🙏 That'd probably help me to understand how to work around my problem.

k0kubun commented 3 years ago

What the comment means is that, if you do this, haml may be used in Tilt usages, depending on an order of require calls. If you don't use Tilt (e.g. through Sinatra), you may not care, but for its users making the behavior deterministic by having this is probably not a bad idea.

I can think of a few alternatives:

  1. Fix Haml's "has some impact on the application behavior" instead. It's possible that some libraries will start requiring haml when you forget this issue. Requiring Haml should not be a problem in the first place. Please describe what behavior you're concerned about.
  2. ~Change the check to use "haml/version" to avoid loading the entire gem there.~ Oops, sorry this doesn't work https://github.com/rtomayko/tilt/blob/master/lib/tilt/haml.rb.
  3. Add an option in Hamlit to skip this code. For backward compatibility, the code should be executed by default.

It seems to me that applying 3 and also filing a ticket in Haml for 1 are sensible.

Note that neither 3 nor your patch solves your problem if you use Tilt. Did you confirm your application behavior is fixed by this patch? If so, I'm okay to have 3.

igor-drozdov commented 3 years ago

Please describe what behavior you're concerned about.

It's quite complicated 😅 When I've upgraded Gon gem from 6.2.0 to 6.4.0, the tests failed because element-attrs, like style in:

%li{ style: "text-indent: #{[index * 16, 60].min}px;" }= link

got wrapped in single quotes, like <li style='text-indent: 16px'> instead of <li style="text-indent: 16px">.

The tests become green when either:

  1. These changes are reverted: https://github.com/gazay/gon/commit/e3c25006ceadc3e2758ad1e89fde0aabde939282
  2. When haml_rails is removed from the project
  3. The changes in this PR are applied

I've noticed that haml is no longer a dependency and thought that cleaning up the requiring code would be an easy-win, but since there's a reason why the code exists, I should just continue the investigation of the initial problem in my application

Thank you for having a look and suggesting the options! I'll go ahead and close this PR 👍

igor-drozdov commented 3 years ago

Hm, the application specifies:

Hamlit::RailsTemplate.set_options(attr_quote: '"')

When I add

Haml::Template.options[:attr_wrapper] = '"'

It fixes the tests, but I'm just afraid that haml is used in this case, instead of performant hamlit 🤔

k0kubun commented 3 years ago

Thanks for clarification. If you still need to keep haml as a dependency and you need my help to fix it, I'd appreciate a minimal GitHub repository that reproduces the issue.