Haml 6 attribute behavior change breaks a lot of existing code in sometimes subtle ways

[mak-dunkelziffer]

I use many custom attributes to attach JS behavior to HTML elements. Sometimes I want to deactivate this JS behavior dynamically and use %div(my-custom-attribute-with-attached-js-behavior=false). In Haml 5 this didn't render the attribute. In Haml 6 it does and thereby suddenly activates the JS.

It would be nice if there would be a setting to get back the old behavior or at least remedy the necessity to modify ALL of the JS hooks. And out of curiosity: what were the reasons for this behavior change?

My general thoughts about attribute handling:

• I am fine with the fact that Haml now makes a distinction between boolean and common attributes. This is justified by the HTML spec
• I am fine with Haml casting the values of boolean attributes to true or false
• It seems like Haml is casting the values of all other attributes to a string. I think it would be nice if Haml would NOT "blindly" cast boolean values here or at least provide a setting to turn this off. If a common attribute has a value out of true, false or nil, I most definitely don't want these values to be cast to a string.

What is your opinion on this proposal?

[k0kubun]

Released v6.2.2 that allows you to add custom attributes to Haml::BOOLEAN_ATTRIBUTES. You may use Haml::BOOLEAN_ATTRIBUTES << "my-custom-attribute-with-attached-js-behavior" to achieve what you want. Note that this needs to be executed before templates get compiled (it may or may not impact already-compiled templates).

[mak-dunkelziffer]

Thank you for your fast response and thank you for providing an official way to mark attributes as boolean. We did that via a monkey patch so far:

Haml::AttributeBuilder.class_eval do
  remove_const(:BOOLEAN_ATTRIBUTES).tap do |boolean_attributes|
    const_set(:BOOLEAN_ATTRIBUTES, [*boolean_attributes, *additional_boolean_attributes].freeze)
  end
end

However, this does not fully solve the problem. If there are 100+ different JS components - each attaching to its own custom attribute - I'd have to find all of them and add them to this list. This would be cumbersome, but a one-time task. That's doable. However, I'd also have to keep this list updated whenever I add or remove new JS components. This would be highly susceptible to human error.

I know that I'm asking for an additional if-clause in Haml::AttributeCompiler#compile_common!, which seems very central to the render logic and therefore might be a performance hit. Still, I think the backwards compatibility might be worth it if the performance hit is small enough.

I'm thinking of something like this:

def compile_common!(temple, key, values)
  if LEGACY_BEHAVIOUR
    compile_boolean!(temple, key, values)
  else
    temple << [:html, :attr, key, [:fescape, @escape_attrs, values.last]]
  end
end

I haven't checked, whether that breaks anything else and if that even fixes all our edge cases. Treat this as a draft.

• Would you generally be willing to integrate this?
• When replacing Haml 5 with Hamlit, has the test suite also been replaced? If so, would it be possible to run this against the old tests, to see, whether this is actually backwards compatible? If not, ignore this point.
• What's the performance cost in your benchmarks with LEGACY_BEHAVIOUR = false?

[k0kubun]

Would you generally be willing to integrate this?

No, because this is a trade-off we deliberately made for performance in Haml 6.

When replacing Haml 5 with Hamlit, has the test suite also been replaced? If so, would it be possible to run this against the old tests, to see, whether this is actually backwards compatible? If not, ignore this point.

Hamlit has run the tests of Haml 5 even before it gets merged into Haml, and it has skipped deliberately failed tests. This behavior would be one of them.

What's the performance cost in your benchmarks with LEGACY_BEHAVIOUR = false?

It varies. No impact, as slow as Haml 5, or slower than Haml 5, depending on the benchmark.

---

I'd like to suggest a couple of alternatives.

1. Rename your custom attributes to data-* (or aria-*).
   • This would be more compliant with the HTML spec than what you're currently doing. data attributes are the standard way to achieve what you want in the JS world. I'm not convinced the Haml project should sacrifice its performance and maintainability for supporting malformed HTML.
2. Define a helper to create a hash with optional keys and use it with %div{ hash } syntax
   • e.g. def custom_attrs(**args) = args.compact and %div{ custom_attrs('my-custom-attribute-with-attached-js-behavior': false) }

The benefit of these two ideas is that it doesn't interfere with the performance of other places. Your LEGACY_BEHAVIOUR = true would make everything slower, but my ideas keep the performance of places that don't use custom attributes.

[mak-dunkelziffer]

Well, if the if itself is such a huge performance impact, I can understand your decision. Wouldn't that get optimized away by YJIT though, if the condition is a constant and is always false for people who don't want this behavior?

Thanks for the workaround suggestions. We already considered those and will definitely use them for all new projects. Can't undo decisions of the past, though.

[k0kubun]

Well, if the if itself is such a huge performance impact, I can understand your decision. Wouldn't that get optimized away by YJIT though, if the condition is a constant and is always false for people who don't want this behavior?

This is not as simple as a single runtime if check. When LEGACY_BEHAVIOUR is true, it would generate or run the unoptimized version of the rendering logic. It's beyond the scope of a JIT to change the logic of applications. (edit: I thought you were talking about the true case. Please disregard this part)

There are still a couple of problems:

1. On AttributeBuilder, LEGACY_BEHAVIOUR is checked at runtime. YJIT could optimize it, but it's still slower on the interpreter.
2. It's not just compile_common!. You need to modify multiple places, and it makes the maintenance hard. You also need to be backward-compatible, so I'm very reluctant to add a new interface like that.

All in all, I don't think it's worth the extra maintenance effort when the "workaround" is more performant than the proposed solution for your application and the sole purpose of Haml 6 was to improve performance.

Right now, the only benefit of upgrading Haml to 6 is performance improvements, and you're not gonna get that if you use LEGACY_BEHAVIOUR = false. It defeats the purpose.

Can't undo decisions of the past, though.

I personally recommend the migration with (2) if (1) seems too hard. You can craft a gsub call that matches %div(my-custom-attribute-with-attached-js-behavior=false) and converts that to %div{ custom_attrs('my-custom-attribute-with-attached-js-behavior': false }. It works in any Haml versions, so you can do it before upgrading Haml to 6, and then upgrade Haml to 6.

[mak-dunkelziffer]

The additional maintenance to keep the legacy behavior working is a very good point. We will do the workaround on our side.

Thank you for your patience. Highly appreciated!