Repository: yippee-fun/phlex (ruby)

Issue:
Provide an official attributes hook
A few people have expressed an interest in overriding `__attributes__` in order to prepare or validate their tag attributes in some unique way. This is not a supported use-case and was recently broken by #459.

I’m opening this issue to explore the possibility of exposing an official hook like our hooks for `before_template`, `after_template` and `around_template`.

### Performance

Processing a Hash of attributes is by far the slowest thing in Phlex. To manage this, we store a cache of the processed attributes on the first render of any view class. We can then take advantage of this cache in subsequent renders if the attributes are consistent, which is usually the case.

Any hook should ideally happen after the cache check, with any customisations of the attributes included in the cache.

Additionally, adding this hook to Phlex should not slow down normal renders that don't implement the hook.

### Safety

Phlex uses the return value of `__attributes__` in the HTML output, which means if you override `__attributes__` and return a `String` rather than calling and returning `super`, you bypass important HTML-safety checks. Any official attributes hook must not leave the application vulnerable if `super` is not called and returned by the override.


Discussion:
I have a few uses for this...

1. Be able to side load assets (js, css, etc) whenever specific html classes or other attributes are used. 

2. Add/remove/modify attributes based on other attributes. Kind of like shortcuts

Because these are based on attributes used and will modify the attributes, using the around template hooks won't do the trick really. 

Hope that helps
Just trying a few things out here, so please be gentle 😁

```ruby
class Phlex::HTML
  private def __fetch_attributes__(**attributes)
    attributes = resolve_attributes(**attributes) if respond_to?(:resolve_attributes)
    Phlex::ATTRIBUTE_CACHE[attributes.hash] || __attributes__(**attributes)
  end
end
```

With the above, anywhere you do `Phlex::ATTRIBUTE_CACHE[attributes.hash] || __attributes__(**attributes)`, replace with `__fetch_attributes__(**attributes)`. Then define `resolve_attributes` in your view, making sure to return the attributes.

This works (and fixes #489) , but unfortunately `resolve_attributes` will be called every single time. So would need to find a way of caching that.

The problem I see is the use of `attributes.hash` as the cache key. This should be used after the attributes have been resolved, otherwise it will be incorrect.

So what else could we use for the cache key? A UUID for each element instance?

Thoughts?

>The problem I see is the use of attributes.hash as the cache key. This should be used after the attributes have been resolved, otherwise it will be incorrect.

If the goal is to prevent `resolve_attributes` from needing to be called every time, wouldn't you want to base it off the `attributes.hash` _before_ resolving them? The assumption would then have to be that `resolve_attributes` is deterministic, always returning the same output for a given input.

I took a swing at this just for fun. If we changed the arguments to `__attributes__` (and the corresponding final method) to accept a key and the attributes, then we can keep the original attribute's hash key as the cache key.

```ruby
private def __fetch_attributes__(**attributes)
  key = attributes.hash
  Phlex::ATTRIBUTE_CACHE.fetch(key) do
    attributes = resolve_attributes(**attributes) if respond_to?(:resolve_attributes)
    __attributes__(key, **attributes)
  end
end
```

So this will look up based on the provided attributes, and cache the resolved attributes. `__attributes__` is then changed to use that instead of the hash of the provided attributes:

```ruby
private def __attributes__(key, **attributes)
  __final_attributes__(key, **attributes).tap do |buffer|
    Phlex::ATTRIBUTE_CACHE[key] = buffer.freeze
  end
end
```

And the final method just ignores that argument

```ruby
private def __final_attributes__(_key, **attributes)
```

I tried this out on my fork here 1046eddcd90387010c78647556ea5c8e234354e7 and tests are passing, but I haven't done any performance testing on it yet.

Regarding the _need_ for something like this for me, I haven't run into a case where I've needed this functionality. But I can see the potential. I would vote that I value performance over supporting this use case— a vote which is influenced primarily by me not having a need for it 😇 yet?
My expectation is that the hash should be based on the final attributes used - after resolving the hook. Otherwise it's not "correct". 

I also tried this in my [branch](https://github.com/joelmoss/phlex/tree/resolve_attributes_hook_with_key), but my tests failed...

```ruby
describe 'resolve_attributes hook' do
  describe 'first' do
    view1 = Class.new(Phlex::HTML) do
      def resolve_attributes(**attributes)
        attributes[:class] = '1'
        attributes
      end

      def template
        div(identical: 'attribute') { 'one' }
      end
    end

    view2 = Class.new(Phlex::HTML) do
      def resolve_attributes(**attributes)
        attributes[:class] = '2'
        attributes
      end

      def template
        div(identical: 'attribute') { 'two' }
      end
    end

    it 'renders' do
      expect(view1.new.call).to be == %(<div identical="attribute" class="1">one</div>)
      expect(view2.new.call).to be == %(<div identical="attribute" class="2">two</div>)
    end
  end
end
```

The above test calls two views with a different template, but identical attributes. However, each view implements the `resolve_attributes` hook, and adds a `class` attribute, with each view having a different `class` value.

The last expectation fails for view2, as it uses the resolved attributes from view1. Which it should obviously not do.

I have [another branch](https://github.com/joelmoss/phlex/tree/resolve_attributes_hook) which has a slight variation on my original proposal, which does pass tests.
FWIW, this is a blocker for me. So i've been having to use my [branch](https://github.com/joelmoss/phlex/tree/resolve_attributes_hook) which has been working nicely, and doesn't suffer to much of a performance hit.
>My expectation is that the hash should be based on the final attributes used - after resolving the hook. Otherwise it's not "correct".

You're right, to have it be correct when multiple definitions of `resolve_attributes` are in play, then you either need to cache based on the final attributes which means that you always have to call `resolve_attributes`, or you would have to have a different cache for each definition of `resolve_attributes`.

It seems like the best implementation is going to be dependent on how people expect to use it. It sounds like you have a use case where you would be defining `resolve_attributes` potentially many times across different components, as opposed to one "global" definition of it at an `ApplicationView` level. Your implementation is good for your use case because you only incur the additional performance hit on the components where the method is defined, and nowhere else.

It might be instructive to see an example of some of the `resolve_attributes` definitions you're currently using, and what types of components you're defining them on, if you have the time.
Having an attribute cache per view class would solve this. There will be a performance cost. That's okay, but I’m looking for a way to implement it so that it doesn't hurt performance for people who are not using the feature.
@joelmoss I am going to have a proper go at this soon. I’m not promising I’ll find a solution but I will try.
Shout if you need a hand. Right now I'm having to use my own fork to provide this, so would love to stop doing that 🙏
Quick update on this: I found a way to perform optional per-class caching with only a 1% performance overhead on classes that don't opt-in, and about a 10% overhead on classes that do opt-in. 1% is reasonable to me, so I’m happy to proceed with this.

Patch:
diff --git a/lib/phlex/elements.rb b/lib/phlex/elements.rb
--- a/lib/phlex/elements.rb
+++ b/lib/phlex/elements.rb
@@ -20,11 +20,11 @@ def register_element(element, tag: element.name.tr("_", "-"))
 			def __phlex_#{element}__(**attributes, &block)
 				if attributes.length > 0
 					if block_given?
-						@_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || __attributes__(**attributes)) << ">"
+						@_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[respond_to?(:process_attributes) ? (attributes.hash + self.class.hash) : attributes.hash] || __attributes__(**attributes)) << ">"
 						yield_content(&block)
 						@_target << "</#{tag}>"
 					else
-						@_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || __attributes__(**attributes)) << "></#{tag}>"
+						@_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[respond_to?(:process_attributes) ? (attributes.hash + self.class.hash) : attributes.hash] || __attributes__(**attributes)) << "></#{tag}>"
 					end
 				else
 					if block_given?
@@ -53,7 +53,7 @@ def register_void_element(element, tag: element.name.tr("_", "-"))
 
 			def __phlex_#{element}__(**attributes)
 				if attributes.length > 0
-					@_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || __attributes__(**attributes)) << ">"
+					@_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[respond_to?(:process_attributes) ? (attributes.hash + self.class.hash) : attributes.hash] || __attributes__(**attributes)) << ">"
 				else
 					@_target << "<#{tag}>"
 				end
diff --git a/lib/phlex/sgml.rb b/lib/phlex/sgml.rb
--- a/lib/phlex/sgml.rb
+++ b/lib/phlex/sgml.rb
@@ -246,12 +246,16 @@ def capture(&block)
 		# @api private
 		private def __attributes__(**attributes)
 			__final_attributes__(**attributes).tap do |buffer|
-				Phlex::ATTRIBUTE_CACHE[attributes.hash] = buffer.freeze
+				Phlex::ATTRIBUTE_CACHE[respond_to?(:process_attributes) ? (attributes.hash + self.class.hash) : attributes.hash] = buffer.freeze
 			end
 		end
 
 		# @api private
 		private def __final_attributes__(**attributes)
+			if respond_to?(:process_attributes)
+				attributes = process_attributes(**attributes)
+			end
+
 			if attributes[:href]&.start_with?(/\s*javascript:/)
 				attributes.delete(:href)
 			end


Test patch:
diff --git a/test/phlex/view/process_attributes.rb b/test/phlex/view/process_attributes.rb
new file mode 100644
--- /dev/null
+++ b/test/phlex/view/process_attributes.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+describe Phlex::HTML do
+	extend ViewHelper
+
+	view do
+		def template
+			div(class: "foo")
+		end
+
+		def process_attributes(**attributes)
+			attributes.transform_values { |v| "#{v}-bar" }
+		end
+	end
+
+	it "works" do
+		expect(output).to be == %(<div class="foo-bar"></div>)
+	end
+end