search

ViewComponent / view_component

A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
https://viewcomponent.org
MIT License
3.17k stars 404 forks source link

Fix inline component throws error with strip_trailing_whitespace #2015

Closed reeganviljoen closed 1 month ago

reeganviljoen commented 1 month ago

closes #2013

What are you trying to accomplish?

Inline components throws

/Users/<redacted>/view_component/lib/view_component/compiler.rb:251:in `compiled_inline_template': undefined method `rstrip!' for an instance of ViewComponent::InlineTemplate::Template (NoMethodError)

 template.rstrip! if component_class.strip_trailing_whitespace?

when strip_trailing_whitespace is used, so this is an attempt to make strip_trailing_whitespace behave in inline components as they do for normal components

What approach did you choose and why?

I removed template.rstrip! if component_class.strip_trailing_whitespace? as the template object is a ViewComponent::InlineTemplate::Template and not a string I then added template = template.source.dup so tat the template string could be unfrozen for compile_template to remove the trailing white space

Performance Imapct

before change

ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin21]
Warming up --------------------------------------
    inline_component   323.000 i/100ms
Calculating -------------------------------------
    inline_component      3.118k (± 4.8%) i/s -     31.331k in  10.075606s

After

ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin21]
Warming up --------------------------------------
strip_trailing_white_space
                       228.000 i/100ms
Calculating -------------------------------------
strip_trailing_white_space
                          3.057k (± 4.5%) i/s -     30.552k in  10.019824s
reeganviljoen commented 1 month ago

@Spone I have simplified the failing test with while keeping the same error

reeganviljoen commented 1 month ago

it seems strip_trailing_whitespace doesn't work with inline templates, I have created a fix locally that seem to work, I will push it up latter

Spone commented 1 month ago

Awesome, that's really useful!

reeganviljoen commented 1 month ago

@Spone I have updated the description, to reflect the actual bug, I then also fixed the test and have added the fix for the bug

reeganviljoen commented 1 month ago

@Spone I write a benchmark by rewriting the old performance inline_componet on a new component and testing that, it seems this is actually faster by a small margin see the description for more details